Offset shape bugfix #73
Conversation
WalkthroughThe pull request introduces modifications across several configuration files. Key changes include renaming the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
stac_mjx/main.py (2)
98-98: LGTM: Consistent return value and minor suggestionThe final return statement has been updated to use
fit_offsets_path, maintaining consistency with the variable renaming throughout the function. The functionality remains unchanged.Consider updating the function's docstring to reflect the change from
fit_pathtofit_offsets_pathin the return value description. This will ensure complete consistency and improve documentation accuracy.
Inconsistent Renaming of
fit_pathDetectedThe verification revealed that not all instances of
fit_pathhave been renamed tofit_offsets_path. Specifically:
Remaining Instances of
fit_path:
demos/run_stac_fly_model.py: Line withfit_path, transform_path = stac_mjx.run_stac(tests/unit/test_main.py: Line withassert cfg.stac.fit_path == "fit.p"run_stac.py: Multiple lines referencingfit_pathVerified Renamed Instances:
- All occurrences of
fit_offsets_pathinstac_mjx/main.pyare correctly updated.Please address the remaining instances of
fit_pathto ensure consistent renaming across the codebase.🔗 Analysis chain
Line range hint
1-98: Summary: Consistent renaming improves code clarityThe changes in this file primarily involve renaming
fit_pathtofit_offsets_path. This renaming has been consistently applied throughout therun_stacfunction, affecting variable initialization, logging, file operations, and return statements. The changes align with the PR objectives and improve code clarity by using a more descriptive variable name.Key points:
- All occurrences of
fit_pathhave been updated tofit_offsets_path.- The functionality of the
run_stacfunction remains unchanged.- The changes are consistent with the configuration update mentioned in the PR summary.
To ensure the renaming has been applied consistently across the codebase, please run the following verification script:
This script will help ensure that the renaming has been applied consistently across the entire codebase and that no instances of the old naming convention remain.
Action Required: Update Verification Script to Handle File Type Issue
The initial verification script failed because the
pythonfile type is unrecognized. To address this, please use the updated script below that targets Python files by their.pyextension:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all occurrences of 'fit_path' have been renamed to 'fit_offsets_path' # Test 1: Check for any remaining instances of 'fit_path' echo "Checking for remaining instances of 'fit_path':" rg --type python 'fit_path' # Test 2: Verify the usage of 'fit_offsets_path' echo "Verifying usage of 'fit_offsets_path':" rg --type python 'fit_offsets_path' # Test 3: Check for any references to 'cfg.stac.fit_path' echo "Checking for any references to 'cfg.stac.fit_path':" rg --type python 'cfg\.stac\.fit_path' # Test 4: Verify the usage of 'cfg.stac.fit_offsets_path' echo "Verifying usage of 'cfg.stac.fit_offsets_path':" rg --type python 'cfg\.stac\.fit_offsets_path'Length of output: 711
Script:
#!/bin/bash # Description: Verify that all occurrences of 'fit_path' have been renamed to 'fit_offsets_path' # Test 1: Check for any remaining instances of 'fit_path' echo "Checking for remaining instances of 'fit_path' in Python files:" rg 'fit_path' --glob '*.py' # Test 2: Verify the usage of 'fit_offsets_path' echo "Verifying usage of 'fit_offsets_path' in Python files:" rg 'fit_offsets_path' --glob '*.py' # Test 3: Check for any references to 'cfg.stac.fit_path' echo "Checking for any references to 'cfg.stac.fit_path' in Python files:" rg 'cfg\.stac\.fit_path' --glob '*.py' # Test 4: Verify the usage of 'cfg.stac.fit_offsets_path' echo "Verifying usage of 'cfg.stac.fit_offsets_path' in Python files:" rg 'cfg\.stac\.fit_offsets_path' --glob '*.py'Length of output: 1575
stac_mjx/stac.py (3)
Line range hint
416-428: Consider addressing the commented-out FLY_MODEL sectionThere's a commented-out section for handling FLY_MODEL type errors in h5 files. It might be beneficial to either implement this functionality if it's needed or remove the commented code if it's no longer relevant. This will improve code clarity and maintainability.
Would you like assistance in implementing the FLY_MODEL handling or removing the commented code?
Line range hint
1-589: Consider further modularization and addressing TODOs
The
Stacclass handles a wide range of functionality. Consider splitting it into smaller, more focused classes to improve maintainability and readability.Long methods like
fit_offsetsandrendercould be broken down into smaller, more focused methods. This would enhance readability and make the code easier to maintain and test.There are TODO comments in the
fit_offsetsmethod. It would be beneficial to address these items or create issues to track them for future implementation.Would you like assistance in modularizing the class or addressing the TODO items?
Line range hint
1-589: Enhance input validation and error handlingWhile the
rendermethod includes good input validation, other methods likefit_offsetsandik_onlycould benefit from similar checks. Consider adding input validation and consistent error handling across all methods in theStacclass. This would improve the robustness and reliability of the code, making it easier to debug and maintain.Would you like assistance in implementing input validation for the other methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- configs/stac/demo.yaml (1 hunks)
- stac_mjx/main.py (3 hunks)
- stac_mjx/stac.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
configs/stac/demo.yaml (1)
Line range hint
1-12: Verify removal ofik_onlyparameterThe AI summary mentions that
ik_onlywas renamed toskip_ik_only, but the removal ofik_onlyis not visible in the provided code. We need to confirm if this removal occurred.Let's search for any remaining instances of
ik_only:If this search returns results, it might indicate that the renaming was not complete or consistent across all configuration files.
✅ Verification successful
📄 Verification Successful:
ik_onlyParameter RemovedAll instances of the
ik_onlyparameter have been successfully removed and renamed toskip_ik_onlyin the YAML configuration files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ik_only in YAML files rg --type yaml 'ik_only:' configs/Length of output: 210
stac_mjx/main.py (4)
59-59: LGTM: Improved variable namingThe change from
fit_pathtofit_offsets_pathenhances code clarity by using a more descriptive variable name. This is consistent with the renaming mentioned in the PR summary.
72-73: LGTM: Consistent variable usageThe logging statement and save operation have been updated to use
fit_offsets_path, maintaining consistency with the earlier variable renaming. The functionality remains unchanged.
78-78: LGTM: Consistent return valueThe return statement has been updated to use
fit_offsets_path, maintaining consistency with the variable renaming throughout the function. The functionality remains unchanged.
86-86: LGTM: Consistent file path usageThe file opening operation has been updated to use
fit_offsets_path, maintaining consistency with the variable renaming. The functionality remains unchanged.stac_mjx/stac.py (2)
393-393: Approved: Consistent reshaping of offsetsThe change to reshape
self._offsetsto(-1, 3)ensures a consistent 2D array format for the offsets, regardless of their original shape. This improvement aligns with the PR objective of addressing offset reshaping issues and enhances data structure consistency throughout the code.
Line range hint
1-589: Summary of review
- The main change to reshape offsets in the
_package_datamethod has been approved and improves data consistency.- Suggestions for improvement include:
- Addressing the commented-out FLY_MODEL section
- Further modularizing the
Stacclass and breaking down long methods- Addressing TODO comments
- Enhancing input validation and error handling across all methods
Overall, the changes in this PR address the stated objective of fixing offset shape issues. The additional suggestions, if implemented, would further improve the code quality and maintainability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 40.95% 39.93% -1.03%
==========================================
Files 10 10
Lines 586 576 -10
==========================================
- Hits 240 230 -10
Misses 346 346 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
configs/stac/demo.yaml (1)
Line range hint
1-13: Summary of changes and recommendationsThe changes in this configuration file align with the PR objectives and improve clarity. However, there are a few important points to address:
- Ensure that all references to
fit_pathhave been updated tofit_offsets_paththroughout the codebase.- Verify that the change from
ik_onlytoskip_ik_onlyhas been consistently applied across all relevant files.- Review and update any logic that depends on these parameters, especially considering the inversion of the
ik_onlylogic.These changes have the potential to impact the behavior of the system, so thorough testing is recommended to ensure everything functions as expected with the new configuration.
Consider adding a deprecation warning for any code that still uses the old parameter names to ease the transition and catch any missed updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- configs/stac/demo.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
configs/stac/demo.yaml (2)
7-7:⚠️ Potential issueVerify the consistency of
skip_ik_onlyacross the codebase.The change from
ik_onlytoskip_ik_onlyrepresents a logical inversion. While this change improves clarity, it's crucial to ensure that:
- All instances of
ik_onlyhave been replaced withskip_ik_onlythroughout the codebase.- The logic in the code has been updated to reflect this inversion (i.e., where
if ik_onlywas used, it should now beif not skip_ik_only).To verify this change, please run the following script:
Review the output to ensure all instances have been updated correctly and the logic has been adjusted as needed.
1-1: Approve the parameter renaming and verify its usage.The renaming of
fit_pathtofit_offsets_pathimproves clarity. However, ensure that all references to this parameter in the codebase are updated accordingly.Run the following script to verify the parameter usage:
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
configs/stac/stac.yaml (1)
Line range hint
10-16: Suggestion: Update commented FLY_MODEL configurationThe commented FLY_MODEL section contains the outdated
fit_pathkey. Consider updating it tofit_offsets_pathfor consistency with the recent changes. This will ensure that if the FLY_MODEL configuration is uncommented in the future, it will use the correct key names.Here's the suggested change:
##FLY_MODEL -# fit_path: "fit_tethered.p" +# fit_offsets_path: "fit_tethered.p" # transform_path: "transform_tethered.p" # data_path: "tests/data/test_rodent_mocap_1000_frames.nwb" # #n_fit_frames: 601 #skip_fit_offsets: False #skip_ik_only: Falseconfigs/stac/stac_fly_treadmill.yaml (1)
Line range hint
3-7: Consider updating data download instructions.While not directly related to the changes made, the comment about downloading data could be improved for clarity and ease of use.
Consider updating the comment to include:
- A brief description of what the data file contains.
- The expected file name after download.
- Instructions on where to place the downloaded file relative to the project structure.
For example:
# Download fly treadmill dataset: # 1. Visit: https://datadryad.org/stash/dataset/doi:10.5061/dryad.mpg4f4r73 # 2. Download file: https://datadryad.org/stash/downloads/file_stream/3361804 # 3. Rename downloaded file to "wt_berlin_linear_treadmill_dataset.csv" # 4. Place the file in the "tests/data/" directory of this project data_path: "/tests/data/wt_berlin_linear_treadmill_dataset.csv"This will help future users or contributors to set up the environment more easily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- configs/stac/stac.yaml (1 hunks)
- configs/stac/stac_fly_tethered.yaml (1 hunks)
- configs/stac/stac_fly_treadmill.yaml (1 hunks)
- configs/stac/stac_mouse.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
configs/stac/stac_mouse.yaml (1)
Line range hint
2-13: Configuration appears appropriate and consistentThe remaining configuration parameters are unchanged and seem appropriate for the STAC mouse model. The
skip_ik_onlyparameter set toTrueis consistent with the PR objectives mentioning updates to accommodate renamed values. No further changes are required in this file based on the PR objectives and AI summary.configs/stac/stac.yaml (2)
1-1: Approved: Improved naming for better clarity and consistencyThe change from
fit_pathtofit_offsets_pathis a good improvement. It makes the configuration more descriptive and aligns with thefit_offsetsfunction mentioned in the PR objectives. This change also maintains consistency with similar modifications in other configuration files, enhancing overall code readability and maintainability.
Line range hint
7-7: Verify: Confirm theskip_ik_onlyflag settingThe
skip_ik_onlyflag is set toTrue. Please confirm that this is the intended setting for this configuration. If it's meant to beFalseor if it should be configurable, consider updating it accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- tests/configs/stac/test_stac.yaml (1 hunks)
- tests/unit/test_main.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
tests/configs/stac/test_stac.yaml (2)
Line range hint
1-13: Overall, the changes look good with a minor verification needed.The renaming of
fit_pathtofit_offsets_pathimproves clarity and aligns with the PR objectives. The rest of the file remains unchanged and appears to be in order. The only point that needs verification is the consistency ofik_only_pathandskip_ik_onlyusage across the codebase.
2-2: Verify intention:ik_only_pathunchangedWhile
fit_pathwas renamed tofit_offsets_path,ik_only_pathremains unchanged. This might be intentional, but it's worth verifying to ensure consistency across the codebase.Please run the following script to check for consistency:
#!/bin/bash # Description: Verify the usage of 'ik_only_path' and 'skip_ik_only' across the codebase. # Test 1: Check for 'ik_only_path' usage echo "Checking 'ik_only_path' usage:" rg 'ik_only_path' --type yaml --type py # Test 2: Check for 'skip_ik_only' usage echo "Checking 'skip_ik_only' usage:" rg 'skip_ik_only' --type yaml --type py # Test 3: Check for any remaining 'ik_only' (not path) references echo "Checking for remaining 'ik_only' references:" rg '\bik_only\b' --type yaml --type pyThis script will help ensure that the naming conventions for
ik_only_pathandskip_ik_onlyare consistent across the project.tests/unit/test_main.py (1)
21-21: LGTM: Test updated to reflect configuration changes.The change from
cfg.stac.fit_pathtocfg.stac.fit_offsets_pathin the assertion is consistent with the broader changes made across configuration files in this PR. This update ensures that the test accurately reflects the new configuration structure, maintaining the integrity of the test suite.
* fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]>
* fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]>
* fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]>
* Model runs + draws in notebook, no data output * Configs etc - model not yet working. * Disable energy * Offset shape bugfix (#73) * fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]> * Smoke test (#74) * run stac experiment * Fixed yaml * Test fail with bad input. * Should fail. * Corrected input - test should pass. * Update demo.yaml - enable ik_only() * Revert update demo.yaml * IT'S WORKING * Offset shape bugfix (#73) * fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]> * Configs etc - model not yet working. * Offset shape bugfix (#73) * fix offset shape when saving * update demo config * linter * fix demo.yaml * update configs * update config test --------- Co-authored-by: Charles Zhang <[email protected]> * Configs etc - model not yet working. * Fix weird merge. * Clean up synth_model config file. * Remove TIME_BINS (which was a merge accident.) * Fix smoke test. * Fix smoke test. * Clean up. * Fixed root optimization, but still some debug code. * Add root_kp_index * Forgot model yaml. * Reset rodent configs, enable synth config. * Add synth_data smoke test. * Missed data file. * Clean up. * Add root opt keypoint to model configs + clean up. * Clean up. * CR feedback. * Add synth data generation program. * Add comments. --------- Co-authored-by: Charles Zhang <[email protected]> Co-authored-by: Charles Zhang <[email protected]>
Reshape offset when saving from
fit_offsetsAlso updated
run_stac.pyand stac config files to work with renamed valuesSummary by CodeRabbit
fit_pathtofit_offsets_pathacross multiple configuration files.ik_onlywithskip_ik_onlyin relevant configurations to clarify functionality.