Refactor configs #54
Refactor configs #54
Conversation
WalkthroughWell, lemme tell ya, this pull request ain't just a little patch-up; it's a full-blown overhaul of how them configurations are loaded and managed across various files. Instead of messin' around with separate configs for STAC and model settings, everything's been rolled into one neat package. That means less clutter and more straightforward paths for loadin' data and runnin' the show. The changes hit multiple scripts and notebooks, makin' 'em easier to maintain and understand. Now, let's get to the nitty-gritty! Changes
Possibly related PRs
Tip OpenAI O1 model for chat
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
=======================================
Coverage 42.55% 42.56%
=======================================
Files 9 9
Lines 524 531 +7
=======================================
+ Hits 223 226 +3
- Misses 301 305 +4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
stac_mjx/controller.py (1)
Line range hint
225-258: Keep on truckin' with the new config in thefitmethod.Yer method here handles the new
cfgjust fine. But with all these moving parts, better make sure nothing's slipped through the cracks. How 'bout we add some more tests to cover these changes?Would you like me to help draft some additional tests for this method?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- configs/config.yaml (1 hunks)
- demos/api_usage.ipynb (8 hunks)
- demos/viz_usage.ipynb (3 hunks)
- run_rodent.py (2 hunks)
- stac_mjx/controller.py (15 hunks)
- stac_mjx/main.py (2 hunks)
- stac_mjx/utils.py (5 hunks)
- stac_mjx/viz.py (4 hunks)
- tests/configs/config.yaml (1 hunks)
- tests/configs/model/test_rodent_no_kp_names.yaml (1 hunks)
- tests/conftest.py (1 hunks)
- tests/fixtures/configs.py (1 hunks)
- tests/fixtures/datasets.py (1 hunks)
- tests/test_controller.py (1 hunks)
- tests/test_main.py (1 hunks)
- tests/test_utils.py (1 hunks)
Files skipped from review due to trivial changes (2)
- tests/fixtures/configs.py
- tests/fixtures/datasets.py
Additional context used
yamllint
configs/config.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
tests/configs/config.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
Ruff
tests/conftest.py
4-4:
from tests.fixtures.configs import *used; unable to detect undefined names(F403)
tests/test_main.py
14-14: Found useless expression. Either assign it to a variable or remove it.
(B018)
stac_mjx/main.py
13-13:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
stac_mjx/utils.py
27-27: Do not perform function call
Path.cwdin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
stac_mjx/controller.py
141-141: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
415-415: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
441-441: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
GitHub Check: codecov/patch
stac_mjx/viz.py
[warning] 38-38: stac_mjx/viz.py#L38
Added line #L38 was not covered by tests
[warning] 50-50: stac_mjx/viz.py#L50
Added line #L50 was not covered by testsstac_mjx/main.py
[warning] 57-58: stac_mjx/main.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: stac_mjx/main.py#L60
Added line #L60 was not covered by tests
[warning] 62-62: stac_mjx/main.py#L62
Added line #L62 was not covered by tests
[warning] 65-66: stac_mjx/main.py#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 74-74: stac_mjx/main.py#L74
Added line #L74 was not covered by testsstac_mjx/controller.py
[warning] 167-167: stac_mjx/controller.py#L167
Added line #L167 was not covered by tests
[warning] 225-226: stac_mjx/controller.py#L225-L226
Added lines #L225 - L226 were not covered by tests
[warning] 388-388: stac_mjx/controller.py#L388
Added line #L388 was not covered by tests
[warning] 415-415: stac_mjx/controller.py#L415
Added line #L415 was not covered by tests
[warning] 417-417: stac_mjx/controller.py#L417
Added line #L417 was not covered by tests
[warning] 528-528: stac_mjx/controller.py#L528
Added line #L528 was not covered by tests
Gitleaks
demos/viz_usage.ipynb
69-69: Identified a pattern that may indicate AWS credentials, risking unauthorized cloud resource access and data breaches on AWS platforms.
(aws-access-token)
Additional comments not posted (14)
tests/test_controller.py (1)
11-18: Y'all did some fine work simplifying this here test function.The changes to
test_init_stacmake it cleaner than a whistle with that singleconfigparameter. But, let's make sure the new gear is greased up right. Check thatcfgobject is structured as expected in other parts of the codebase, especially where it's loaded and used.Verification successful
Alright, let's fix that busted command and get back on the road. We're gonna run another script to check how that
cfgobject is struttin' its stuff across the codebase. Hang tight, and we'll get this sorted.
Well, I'll be! That
cfgobject is struttin' its stuff just fine across the codebase.Looks like y'all got that
DictConfigstruttin' through the code like a well-oiled rig. Everything's in order, and thattest_init_stacfunction is sittin' pretty with the rest of 'em. No need to worry 'bout any loose bolts here.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg --type python -A 5 $'cfg'Length of output: 58
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg --type py -A 5 $'cfg'Length of output: 17548
tests/test_main.py (1)
11-24: This here test function's been trimmed down like a neat haircut.Good job on streamlining
test_load_configswith that singleconfigparameter. Just make sure thecfgobject ain't just a pretty face but works hard under the hood across the codebase.Verification successful
Alright, let's fix that busted command and get this rig rolling again. Gonna search for that
cfgobject without any fancy file type nonsense. Hang tight, partner.
That
cfgobject is struttin' its stuff just fine, partner.The
cfgobject is struttin' its stuff all over the codebase, and it's struttin' right. It's aDictConfigthrough and through, and it's struttin' in line with them test assertions. Ain't no need to worry 'bout it not pullin' its weight.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg --type python -A 5 $'cfg'Length of output: 58
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg -A 5 'cfg'Length of output: 88549
Tools
Ruff
14-14: Found useless expression. Either assign it to a variable or remove it.
(B018)
run_rodent.py (2)
Line range hint
25-41: This function's been cleaned up like a new pin in a bowling alley.Well done on trimming down
hydra_entryto just the essentials. Make sure the logs are still giving you the full picture and ain't missing any critical details.Verification successful
Alright, let's fix that busted command and get back on the road. Gonna rerun the script with the right file type this time. Hang tight.
Logging's as solid as a rock, partner.
The logging statements are spread out nicely across the codebase, covering all the important stops along the way. The changes in
hydra_entryain't left any gaps in the logs, so you're good to roll on.
run_rodent.py: Logs configuration and completion.stac_mjx/main.py: Logs various stages of the process.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logging and configuration structure across the codebase. # Test: Search for the logging statements and configuration usage. Expect: Consistent and comprehensive logging. rg --type python -A 5 $'logging.info'Length of output: 67
Script:
#!/bin/bash # Description: Verify the logging and configuration structure across the codebase. # Test: Search for the logging statements and configuration usage. Expect: Consistent and comprehensive logging. rg --type py -A 5 $'logging.info'Length of output: 2143
Tools
Ruff
12-12:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
15-18: Y'all streamlined this function like a big rig on a straight highway.Nice work on
load_and_run_stac, making it slicker than snot on a doorknob with thatcfgparameter. Just double-check that this new setup doesn't throw a wrench in the works elsewhere.Verification successful
Alright, partner, looks like we hit a bump in the road with that last command. Let's fix 'er up and get back on track. I'm gonna rerun the search with the right file type for Python and see if we can get a bead on that
cfgobject.
Well, butter my biscuit, that
cfgis struttin' its stuff all over the place like a rooster in a henhouse.Looks like y'all got that
cfgstruttin' through the codebase like it owns the joint. Everything's lookin' consistent and tidy, so no need to worry 'bout any loose bolts flyin' off. Keep on truckin' with those changes, they're as solid as a rock.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg --type python -A 5 $'cfg'Length of output: 58
Script:
#!/bin/bash # Description: Verify the structure of the `cfg` object across the codebase. # Test: Search for the `cfg` object usage. Expect: Consistent structure and usage. rg --type py -A 5 $'cfg'Length of output: 17548
tests/configs/model/test_rodent_no_kp_names.yaml (1)
7-7: Well, lookie here, we got ourselves some KP_NAMES back in the game.Looks like you've uncommented the
KP_NAMESline, which is slicker than snot on a doorknob. Just make sure this change plays nice with the rest of your setup.Run the following script to verify the integration of the
KP_NAMESchange:demos/api_usage.ipynb (1)
32-32: Well, butter my butt and call me a biscuit, you've streamlined the whole dang operation!You've refactored the config loading and data handling to be slicker than a greased pig. Just make sure this new setup doesn't throw any wrenches into the gears.
Run the following script to verify the performance and error handling:
Also applies to: 49-49, 256-256
stac_mjx/controller.py (3)
Line range hint
118-141: All clear on the configuration front in_create_body_sites.The method now uses the
cfgobject to fetch configuration values, which aligns with the new setup. Just keep an eye on any external dependencies that might be affected by these changes.Tools
Ruff
141-141: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
388-388: Good job on keepin' it compatible in_package_data.Usin'
OmegaConf.to_containerto convert the config data ensures that the rest of your code can keep on truckin' without hiccups. Nice touch!Tools
GitHub Check: codecov/patch
[warning] 388-388: stac_mjx/controller.py#L388
Added line #L388 was not covered by tests
Line range hint
37-48: Refactoring looks good, but let's double-check the gears are meshin' right.Y'all consolidated the config parameters into a single
cfgobject, which simplifies the setup. Make sure this change is well integrated across the board, especially in parts of the system that interact with theSTACclass.Verification successful
All systems go, partner! The refactoring of the
STACconstructor is lookin' solid across the board. Them calls intests/test_controller.py,stac_mjx/viz.py, andstac_mjx/main.pyare all in line with the new setup. Looks like the gears are meshin' just fine. Keep on truckin'!Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to STAC constructor match the new signature. rg --type python -A 5 $'STAC('Length of output: 92
Script:
#!/bin/bash # Correct the regex pattern and verify that all calls to STAC constructor match the new signature. rg --type python -A 5 'STAC('Length of output: 92
Script:
#!/bin/bash # Escape the parenthesis in the regex pattern and verify that all calls to STAC constructor match the new signature. rg --type python -A 5 'STAC\('Length of output: 63
Script:
#!/bin/bash # Use the correct file type flag and verify that all calls to STAC constructor match the new signature. rg -t py -A 5 'STAC\('Length of output: 891
demos/viz_usage.ipynb (5)
46-46: Consolidation of Configuration LoadingWell, ain't this a sight for sore eyes! Y'all streamlined the config loading to a single line, which is slicker than snot on a doorknob. This change should make the code easier to manage and less cluttered.
83-83: Data Path AdjustmentWell, butter my butt and call me a biscuit! Changing the data path to
demo_fit.pmight just be using fresher or more relevant data for your visualizations. Good on ya for keeping things up-to-date.
88-88: Function Call SimplificationHot dang! Using a single config object instead of juggling two is like swapping from a double-clutch to an automatic. It’s a smoother ride and less hassle. Good job on cleaning up the function call.
91-91: Consistent Configuration Access for Video PlaybackSwitching to
cfg.model.RENDER_FPSfor the frame rate? That’s slicker than a greased pig! It keeps your config access consistent and tidy. Nice touch, partner.
69-69: False Positive on Security AlertHold your horses! That Gitleaks warning about AWS credentials is as wrong as a left turn in NASCAR. It’s just part of a base64-encoded video, no real secrets spilled here.
Tools
Gitleaks
69-69: Identified a pattern that may indicate AWS credentials, risking unauthorized cloud resource access and data breaches on AWS platforms.
(aws-access-token)
| defaults: | ||
| - stac: demo | ||
| - model: rodent | ||
| - _self_ |
There was a problem hiding this comment.
Configuration looks shipshape, but don't forget the newline at the end, partner.
The configuration using Hydra's features is spot on. However, it's best practice to include a newline at the end of the file to avoid any issues with version control systems or text editors that might get finicky about such things.
Add a newline at the end of the file to fix the issue flagged by yamllint:
- _self_
+ Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaults: | |
| - stac: demo | |
| - model: rodent | |
| - _self_ | |
| defaults: | |
| - stac: demo | |
| - model: rodent | |
| - _self_ | |
Tools
yamllint
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
| defaults: | ||
| - stac: test_stac | ||
| - model: test_rodent | ||
| - _self_ |
There was a problem hiding this comment.
Configuration for testing is on the money, but don't skip out on that newline at the end, buddy.
The test configuration settings are well set up using Hydra's features. Just like the main config, don't forget to add a newline at the end of the file to keep things tidy and avoid potential hiccups with tools that expect it.
Add a newline at the end of the file to fix the issue flagged by yamllint:
- _self_
+ Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaults: | |
| - stac: test_stac | |
| - model: test_rodent | |
| - _self_ | |
| defaults: | |
| - stac: test_stac | |
| - model: test_rodent | |
| - _self_ | |
Tools
yamllint
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
tests/conftest.py
Outdated
| @@ -1,3 +1,4 @@ | |||
| """Provide fixtures for the entire parent directory.""" | |||
|
|
|||
| from tests.fixtures.datasets import * | |||
| from tests.fixtures.configs import * | |||
There was a problem hiding this comment.
Tidy up that import, don't just haul everything over willy-nilly.
Using a wildcard import (from tests.fixtures.configs import *) can lead to confusion and errors down the road. It's better to haul in only what you need. This keeps your namespace clean and your code easier to understand.
Replace the wildcard import with specific imports to avoid potential issues:
-from tests.fixtures.configs import *
+from tests.fixtures.configs import ConfigA, ConfigB # Adjust as needed based on actual usageCommittable suggestion was skipped due to low confidence.
Tools
Ruff
4-4:
from tests.fixtures.configs import *used; unable to detect undefined names(F403)
stac_mjx/utils.py
Outdated
| @@ -24,7 +24,7 @@ def enable_xla_flags(): | |||
| ) | |||
|
|
|||
|
|
|||
| def load_data(file_path: Path, params: Dict, label3d_path=None): | |||
| def load_data(cfg: DictConfig, base_path: Path = Path.cwd()): | |||
There was a problem hiding this comment.
Hold your horses, partner! We got a little hiccup with Path.cwd() in the default arguments.
You've done a bang-up job refactoring load_data to use DictConfig. But, as per the static analysis tool's advice, let's not use Path.cwd() in the default arguments. It's like putting all your eggs in one basket.
Here's a fix to keep things truckin':
-def load_data(cfg: DictConfig, base_path: Path = Path.cwd()):
+def load_data(cfg: DictConfig, base_path: Path = None):
+ if base_path is None:
+ base_path = Path.cwd()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def load_data(cfg: DictConfig, base_path: Path = Path.cwd()): | |
| def load_data(cfg: DictConfig, base_path: Path = None): | |
| if base_path is None: | |
| base_path = Path.cwd() |
Tools
Ruff
27-27: Do not perform function call
Path.cwdin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- stac_mjx/main.py (2 hunks)
- stac_mjx/utils.py (5 hunks)
- stac_mjx/viz.py (3 hunks)
- tests/conftest.py (1 hunks)
- tests/test_main.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- stac_mjx/viz.py
Additional context used
Ruff
tests/conftest.py
3-3:
tests.fixtures.datasets.label3d_mat_fileimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_matimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_nwbimported but unusedRemove unused import
(F401)
5-5:
tests.fixtures.configs.rodent_configimported but unusedRemove unused import
(F401)
6-6:
tests.fixtures.configs.rodent_config_label3dimported but unusedRemove unused import
(F401)
7-7:
tests.fixtures.configs.rodent_config_no_kp_namesimported but unusedRemove unused import
(F401)
8-8:
tests.fixtures.configs.rodent_config_less_kp_namesimported but unusedRemove unused import
(F401)
9-9:
tests.fixtures.configs.configimported but unusedRemove unused import
(F401)
tests/test_main.py
12-12: Found useless expression. Either assign it to a variable or remove it.
(B018)
stac_mjx/main.py
13-13:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
Additional comments not posted (2)
stac_mjx/utils.py (2)
13-13: Y'all added some new imports, I reckon.Looks like you're bringing in
DictConfigfromomegaconfand somejaxlibraries. Make sure these new tools are hitched up right in your setup or requirements file, ya hear?
27-27: Whoa there, partner! We got some changes to wrangle inload_data.Y'all have done a number on this function, switching gears to use
DictConfigand reworking the path handling. Here's a rundown:
Default Argument Fix: Good on ya for fixing that
Path.cwd()in the default arguments. That's following the advice from the static analysis tool to a T.Path Handling: You're constructing
file_pathfrombase_pathandcfg.stac.data_path. Just make sure thatcfgstructure is solid as a rock across all your configs.File Type Handling: You're checking file suffixes and loading data accordingly. Just keep an eye out for any new file types you might want to support in the future.
Error Handling: You're throwing a
ValueErrorif the file type ain't supported. That's tighter than a bull's hide on a cold morning. Good job!Keypoint Name Checks: You're ensuring the keypoint names match up with the data. This is crucial for avoiding mix-ups down the line.
Data Scaling and Sorting: You're scaling and sorting the data based on the model configuration. This is slicker than owl snot, ensuring everything lines up for processing.
Overall, you're steering this function in the right direction. Just keep these wheels greased, and you'll be cruising.
Also applies to: 49-84
tests/conftest.py
Outdated
| from tests.fixtures.datasets import label3d_mat_file, mocap_mat, mocap_nwb | ||
| from tests.fixtures.configs import ( | ||
| rodent_config, | ||
| rodent_config_label3d, | ||
| rodent_config_no_kp_names, | ||
| rodent_config_less_kp_names, | ||
| config, | ||
| ) |
There was a problem hiding this comment.
Y'all gotta clean up them imports, partner.
Looks like you've done a bang-up job switchin' from that messy wildcard import to specific ones, which is slicker'n snot on a doorknob. But, hold your horses, 'cause it seems like you've gone overboard and imported a whole bunch of stuff you ain't usin'.
Here's what you need to do:
- Yank out them unused imports to keep your code cleaner than a whistle.
Apply this diff to remove the unused imports:
-from tests.fixtures.datasets import label3d_mat_file, mocap_mat, mocap_nwb
-from tests.fixtures.configs import rodent_config, rodent_config_label3d, rodent_config_no_kp_names, rodent_config_less_kp_names, configCommittable suggestion was skipped due to low confidence.
Tools
Ruff
3-3:
tests.fixtures.datasets.label3d_mat_fileimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_matimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_nwbimported but unusedRemove unused import
(F401)
5-5:
tests.fixtures.configs.rodent_configimported but unusedRemove unused import
(F401)
6-6:
tests.fixtures.configs.rodent_config_label3dimported but unusedRemove unused import
(F401)
7-7:
tests.fixtures.configs.rodent_config_no_kp_namesimported but unusedRemove unused import
(F401)
8-8:
tests.fixtures.configs.rodent_config_less_kp_namesimported but unusedRemove unused import
(F401)
9-9:
tests.fixtures.configs.configimported but unusedRemove unused import
(F401)
| def test_load_configs(config): | ||
| # Check that utils.params is not defined before loading | ||
| with pytest.raises(AttributeError): | ||
| utils.params | ||
|
|
||
| # Call the function | ||
| stac_cfg, model_cfg = main.load_configs( | ||
| _BASE_PATH / stac_config, _BASE_PATH / rodent_config | ||
| ) | ||
| cfg = main.load_configs(config) | ||
|
|
||
| # Assert that the configs are the correct type | ||
| assert isinstance(stac_cfg, DictConfig) | ||
| assert isinstance(model_cfg, Dict) | ||
| assert isinstance(cfg, DictConfig) | ||
|
|
||
| # Assert that the resulting configs contain the expected data | ||
| assert stac_cfg.fit_path == "fit.p" | ||
| assert model_cfg["N_FRAMES_PER_CLIP"] == 360 | ||
| assert cfg.stac.fit_path == "fit.p" | ||
| assert cfg.model.N_FRAMES_PER_CLIP == 360 |
There was a problem hiding this comment.
Well butter my butt and call me a biscuit, this here's some tidy work!
You've streamlined that test_load_configs function slicker than a greased pig. But, I reckon there's a bit of a hiccup with a useless expression floatin' around like a tumbleweed. Make sure every line of code is pullin' its weight.
Here's a fix to remove that useless expression:
- with pytest.raises(AttributeError):
- utils.paramsCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_load_configs(config): | |
| # Check that utils.params is not defined before loading | |
| with pytest.raises(AttributeError): | |
| utils.params | |
| # Call the function | |
| stac_cfg, model_cfg = main.load_configs( | |
| _BASE_PATH / stac_config, _BASE_PATH / rodent_config | |
| ) | |
| cfg = main.load_configs(config) | |
| # Assert that the configs are the correct type | |
| assert isinstance(stac_cfg, DictConfig) | |
| assert isinstance(model_cfg, Dict) | |
| assert isinstance(cfg, DictConfig) | |
| # Assert that the resulting configs contain the expected data | |
| assert stac_cfg.fit_path == "fit.p" | |
| assert model_cfg["N_FRAMES_PER_CLIP"] == 360 | |
| assert cfg.stac.fit_path == "fit.p" | |
| assert cfg.model.N_FRAMES_PER_CLIP == 360 | |
| def test_load_configs(config): | |
| # Call the function | |
| cfg = main.load_configs(config) | |
| # Assert that the configs are the correct type | |
| assert isinstance(cfg, DictConfig) | |
| # Assert that the resulting configs contain the expected data | |
| assert cfg.stac.fit_path == "fit.p" | |
| assert cfg.model.N_FRAMES_PER_CLIP == 360 |
Tools
Ruff
12-12: Found useless expression. Either assign it to a variable or remove it.
(B018)
stac_mjx/main.py
Outdated
| def load_configs(config_dir: Union[Path, str]) -> DictConfig: | ||
| """Initializes configs. | ||
|
|
||
| Args: | ||
| stac_config_path (str): path to stac yaml file | ||
| model_config_path (str): path to model yaml file | ||
| config_dir ([Path, str]): absolute path to config directory | ||
|
|
||
| Returns: | ||
| DictConfig: stac.yaml config to use in run_stac() | ||
| """ | ||
| return OmegaConf.load(stac_config_path), OmegaConf.to_container( | ||
| OmegaConf.load(model_config_path), resolve=True | ||
| ) | ||
| # Initialize Hydra and set the config path | ||
| with hydra.initialize_config_dir(config_dir=str(config_dir), version_base=None): | ||
| # Compose the configuration by specifying the config name | ||
| cfg = hydra.compose(config_name="config") | ||
| return cfg |
There was a problem hiding this comment.
Hot dang, you're on the right track, but don't let the details slip through like a loose load!
You've done a fine job simplifying these functions with Hydra, making 'em as neat as a pin. But, like leavin' tools on the highway, you've got an unused import clutterin' up your space.
Here's how to clean up that mess:
-from typing import List, Dict, Union
+from typing import List, UnionAlso applies to: 34-77
stac_mjx/utils.py
Outdated
| @@ -24,7 +24,7 @@ def enable_xla_flags(): | |||
| ) | |||
|
|
|||
|
|
|||
| def load_data(file_path: Path, params: Dict, label3d_path=None): | |||
| def load_data(cfg: DictConfig, base_path=None): | |||
There was a problem hiding this comment.
Hold your horses on that base_path default!
Looks like you've got base_path=None as a default, which is a smart move following the previous hiccup with Path.cwd(). Just make sure this change is reflected everywhere load_data is called, or you'll be chasing your tail fixing bugs.
| if base_path is None: | ||
| base_path = Path.cwd() | ||
|
|
||
| file_path = base_path / cfg.stac.data_path |
There was a problem hiding this comment.
Tighten up the logic in load_data, cowboy!
You've got a lot of moving parts here, and while it's mostly looking good, here are a few spots to oil up:
-
Path Construction: Make sure
cfg.stac.data_pathis always provided, or you'll be up the creek without a paddle. -
Label Path Handling: The handling of
label3d_pathusingcfg.model.getis slick, but ensure that fallback mechanism is clear as mud to everyone using this. -
Keypoint Name Fallback: Using
kp_names or cfg.model.KP_NAMESis a neat trick. Just make surecfg.model.KP_NAMESis always stocked up. -
Data Sorting and Scaling: The way you're sorting and scaling the data is as neat as a pin. Just double-check those indices and scale factors to avoid any slip-ups.
Keep these gears grinding smoothly, and you'll have a well-oiled machine.
Also applies to: 55-55, 64-64, 77-84
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
stac_mjx/main.py (1)
34-52: Lack of Test Coverage forrun_stacFunctionLooks like the
run_stacfunction ain't got no test coverage, partner. You might wanna hitch up some tests to cover those new lines, just like the review comment suggested. Make sure you get those tests rollin' to cover your bases.Analysis chain
Streamline
run_stacfunctionYou've streamlined the
run_stacfunction by consolidating the config parameters. That's like switchin' from a clunky old transmission to a smooth automatic. Just make sure you've got your tests coverin' these new lines, as the codecov patch is showin' some gaps.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new lines in run_stac function. # Test: Search for test coverage. Expect: Coverage for lines 50-52, 58-59, 61, 63, 66-67, 75. rg --type python -A 5 $'test_run_stac'Length of output: 68
Script:
#!/bin/bash # Corrected script to search for test functions related to run_stac. # Search for any test functions that might cover run_stac. rg --type py -A 5 'def test_' --glob '*test*.py'Length of output: 2484
Script:
#!/bin/bash # Search for calls to run_stac in the test files to check for indirect testing. rg --type py 'run_stac' --glob '*test*.py'Length of output: 42
Tools
GitHub Check: codecov/patch
[warning] 50-51: stac_mjx/main.py#L50-L51
Added lines #L50 - L51 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- stac_mjx/controller.py (17 hunks)
- stac_mjx/main.py (1 hunks)
- stac_mjx/utils.py (5 hunks)
Additional context used
Ruff
stac_mjx/main.py
13-13:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
stac_mjx/utils.py
11-11:
ndx_pose.PoseEstimationSeriesimported but unusedRemove unused import
(F401)
11-11:
ndx_pose.PoseEstimationimported but unusedRemove unused import
(F401)
stac_mjx/controller.py
137-137: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
408-408: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
434-434: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
GitHub Check: codecov/patch
stac_mjx/main.py
[warning] 50-51: stac_mjx/main.py#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 58-59: stac_mjx/main.py#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 61-61: stac_mjx/main.py#L61
Added line #L61 was not covered by tests
[warning] 63-63: stac_mjx/main.py#L63
Added line #L63 was not covered by tests
[warning] 66-67: stac_mjx/main.py#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 75-75: stac_mjx/main.py#L75
Added line #L75 was not covered by testsstac_mjx/controller.py
[warning] 163-163: stac_mjx/controller.py#L163
Added line #L163 was not covered by tests
[warning] 221-222: stac_mjx/controller.py#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 381-381: stac_mjx/controller.py#L381
Added line #L381 was not covered by tests
[warning] 408-408: stac_mjx/controller.py#L408
Added line #L408 was not covered by tests
[warning] 410-410: stac_mjx/controller.py#L410
Added line #L410 was not covered by tests
[warning] 521-521: stac_mjx/controller.py#L521
Added line #L521 was not covered by tests
Additional comments not posted (5)
stac_mjx/main.py (1)
17-30: Refactor: Load configs with HydraYou've done a slick job switchin' to Hydra for config management. This change simplifies the whole setup and makes it easier to manage with
config_diras a single input. Just make sure all parts of your rig are tuned to handle this new setup.stac_mjx/utils.py (1)
Line range hint
27-84: Refactor:load_datafunctionYou've reworked the
load_datafunction to be more flexible withDictConfig. That's like upgrading your rig with a new navigation system. Just make sure all the paths and configurations are correctly set up in your tests to avoid any detours.stac_mjx/controller.py (3)
Line range hint
84-253: Optimize part setup and transformationsYou've got some fine adjustments here, making sure each part is optimized just right. That's like fine-tuning your rig's suspension for a smoother ride. Keep an eye on those test coverages though, as some parts might not be fully checked under load.
381-381: Pack data efficientlyYou're packing data as neatly as a trucker's cab. Using
OmegaConf.to_containerto ensure compatibility is a smart move, like using the right tool for the right job.Tools
GitHub Check: codecov/patch
[warning] 381-381: stac_mjx/controller.py#L381
Added line #L381 was not covered by tests
Line range hint
37-69: Refactor:STACclass constructorYou've streamlined the
STACclass constructor by consolidating the config parameters. That's like tuning up your engine for better performance. Just make sure all the gears mesh well with this new setup.Verification successful
Alright, let's get this rig back on the road and see if we can find those tests. I'm gonna run a new script to sniff out any Python test files and see if they're covering that
STACclass constructor. Hang tight, partner.
Well, looks like we hit a few bumps on the road with those errors, but we did find a test function called
test_init_stacintests/test_controller.py. That might just be the ticket for checking theSTACclass constructor. Let's take a closer look at that test function to see if it's got the coverage we need.
Well, partner, looks like we got ourselves a solid test for that
STACclass constructor. Thetest_init_stacfunction is checkin' all the right boxes, makin' sure everything's hooked up just right. So, we're good to go on that front. Keep on truckin'!
- Test Coverage Confirmed: The
STACclass constructor is covered by thetest_init_stacfunction intests/test_controller.py.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new lines in STAC class constructor. # Test: Search for test coverage. Expect: Coverage for lines 163, 221-222, 381, 408, 410, 521. rg --type python -A 5 $'test_STAC_constructor'Length of output: 76
Script:
#!/bin/bash # Find all Python test files and search for test functions related to the STAC class constructor. fd --type f --extension py --exec rg 'def test_' {} +Length of output: 1357
Script:
#!/bin/bash # Check the content of the test function `test_init_stac` in `tests/test_controller.py`. rg 'def test_init_stac' -A 20 tests/test_controller.pyLength of output: 409
stac_mjx/main.py
Outdated
| @@ -10,66 +10,69 @@ | |||
| from stac_mjx import utils | |||
| from stac_mjx.controller import STAC | |||
| from pathlib import Path | |||
| from typing import List, Dict | |||
| from typing import List, Dict, Union | |||
There was a problem hiding this comment.
Yank out that unused import, partner!
Looks like you're haulin' some extra weight with that typing.Dict import. Ain't no use for it in this file, so best to cut it loose.
-from typing import List, Dict, Union
+from typing import List, UnionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import List, Dict, Union | |
| from typing import List, Union |
Tools
Ruff
13-13:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
stac_mjx/controller.py (1)
Line range hint
37-69: Rev up your engines, but don't skip the maintenance!You've done a bang-up job simplifying the
STACclass constructor by consolidatin' them config parameters. But, like a truck without its mud flaps, you're missin' some coverage on them new lines.# Suggestion: Add tests for the new lines in the `STAC` constructor to ensure they behave as expected.Need help draftin' them tests? I can whip up some test cases or open a GitHub issue to track this task.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- run_rodent.py (2 hunks)
- stac_mjx/controller.py (17 hunks)
- stac_mjx/main.py (1 hunks)
Additional context used
GitHub Check: codecov/patch
stac_mjx/main.py
[warning] 50-51: stac_mjx/main.py#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 58-59: stac_mjx/main.py#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 61-61: stac_mjx/main.py#L61
Added line #L61 was not covered by tests
[warning] 63-63: stac_mjx/main.py#L63
Added line #L63 was not covered by tests
[warning] 66-67: stac_mjx/main.py#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 75-75: stac_mjx/main.py#L75
Added line #L75 was not covered by testsstac_mjx/controller.py
[warning] 163-163: stac_mjx/controller.py#L163
Added line #L163 was not covered by tests
[warning] 221-222: stac_mjx/controller.py#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 381-381: stac_mjx/controller.py#L381
Added line #L381 was not covered by tests
[warning] 408-408: stac_mjx/controller.py#L408
Added line #L408 was not covered by tests
[warning] 410-410: stac_mjx/controller.py#L410
Added line #L410 was not covered by tests
[warning] 521-521: stac_mjx/controller.py#L521
Added line #L521 was not covered by tests
Ruff
stac_mjx/controller.py
137-137: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
408-408: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
434-434: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (5)
run_rodent.py (2)
Line range hint
24-40: This ain't your first rodeo, huh? Good job on cutting the fat.The switch to a single
cfgparameter and streamlined logging is as neat as a pin. Just make sure the logging ain't too sparse for tracking down issues in the wild.Verification successful
Alright, let's fix that busted command and take another crack at it. Gonna run a new script to sniff out that
hydra_entryfunction and see if the logging's up to snuff.
Logging's as good as a fresh set of tires, partner. That
hydra_entryfunction's got its logging ducks in a row, showing the whole configuration in YAML. Shouldn't have any trouble tracking down issues with that setup.
- The logging captures the configuration using
OmegaConf.to_yaml(cfg), which is detailed enough for debugging.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logging details in `hydra_entry`. # Test: Search for the function usage. Expect: Sufficient logging details for debugging. rg --type python -A 5 $'hydra_entry'Length of output: 66
Script:
#!/bin/bash # Description: Verify the logging details in `hydra_entry`. # Test: Search for the function usage in Python files. Expect: Sufficient logging details for debugging. rg -t py -A 5 $'hydra_entry'Length of output: 433
14-17: Y'all got this streamlined, but let's double-check the gears.The simplification to a single
cfgparameter looks slicker than snot on a doorknob. Just make sure all parts of the code are greased up to handle this newcfgstructure right.stac_mjx/main.py (2)
13-13: Yank out that unused import, partner!Looks like you're haulin' some extra weight with that
typing.Dictimport. Ain't no use for it in this file, so best to cut it loose.-from typing import List, Dict, Union +from typing import List, Union
17-30: Tighten up them bolts!You've done a slick job simplifying these functions with Hydra, making 'em as neat as a pin. But, like leavin' tools on the highway, you've got an unused import clutterin' up your space.
-from typing import List, Dict, Union +from typing import List, UnionRun the following script to verify the function usage:
Verification successful
Alright, partner, looks like we hit a bump in the road with that fancy
rgcommand. Let's get back on track and find where thisload_configsis bein' used. I'm gonna whip up a new script to sniff out its trail without fussin' over file types.
Keep on truckin'!
Well, partner, looks like you got this rig runnin' smooth with that
load_configsrefactor. The new signature's hittin' all the right spots across the codebase, from tests to demos. Just make sure them tests keep passin' and you'll be haulin' a full load of success.
- README.md: Example usage aligns with the new signature.
- tests/test_main.py: Test cases validate the function's behavior.
- tests/test_controller.py: Further usage in tests.
- demos/viz_usage.ipynb and demos/api_usage.ipynb: Demos use the updated function.
Keep an eye on them tests, and you'll be rollin' down the highway just fine.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `load_configs` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'load_configs'Length of output: 67
Script:
#!/bin/bash # Description: Verify all function calls to `load_configs` match the new signature. # Test: Search for the function usage without specifying file type. rg -A 5 'load_configs'Length of output: 2833
stac_mjx/controller.py (1)
84-98: Tighten up them gears!You've got a solid setup for part optimization, but don't let them static analysis warnings slip by. Clean up your code by removin' unnecessary
.keys()and fixin' that unused loop variable.- for key in self.cfg.model.KEYPOINT_MODEL_PAIRS.keys() + for key in self.cfg.model.KEYPOINT_MODEL_PAIRS- for id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS): + for _id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS):Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
README.md (1)
Line range hint
67-86: Watch out for them import statements, old timer.You're importing
stac_mjxtwice in different sections (lines 67 and 41). Consolidate them imports at the top of your example to keep things neat and tidy. Also, make suremediapyand other dependencies are listed in your environment setup if they're needed for running the examples.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- README.md (3 hunks)
- run_stac.py (1 hunks)
Additional comments not posted (4)
run_stac.py (3)
29-30: All set to hit the road with this main block.This main execution block looks clean and clear. Just make sure
hydra_entry()is robust against different execution contexts, especially if this script is gonna be part of a larger application or service.
20-27: Tighten up that Hydra config, buckaroo.You're using Hydra mighty fine here, but let's make sure you're pointing to the right config directory. If
./configsis not the root, you might wanna adjust that path based on where this script is run from, or use an absolute path. Also, check ifversion_base=Noneis what you really want, or if it's just a leftover from copy-pasting.
24-24: Good on ya for setting them XLA flags.Just make sure this method call is necessary every time the script runs, especially if it's gonna be used in environments without GPUs. Might wanna conditionally enable these based on the presence of compatible hardware.
Verification successful
No need to worry, partner, them XLA flags are already in check. The
enable_xla_flagsfunction instac_mjx/utils.pymakes sure them flags only get set if there's a GPU around. So, you ain't gotta fret about it runnin' in places without the right hardware.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if GPU is available for XLA if lspci | grep -i nvidia; then echo "GPU found, XLA can be enabled." else echo "No GPU found, consider conditional XLA flag setting." fiLength of output: 212
Script:
#!/bin/bash # Search for the implementation of the enable_xla_flags method in the codebase ast-grep --lang python --pattern 'def enable_xla_flags($$$_):'Length of output: 64
Script:
#!/bin/bash # Search for any references or definitions of enable_xla_flags in the codebase rg 'enable_xla_flags' -A 5Length of output: 1215
README.md (1)
38-58: Hold your horses on these usage instructions.Y'all did a bang-up job updating these instructions, but make sure the paths and script names are up-to-date with the latest directory structure and naming conventions. Also, double-check that all links, especially to external documentation like Hydra, are working and lead to the correct pages.
Verification successful
Paths are all clear, but keep an eye on those links.
The script and config paths in the README are spot on, partner. Just make sure to double-check that the link to the Hydra documentation is still hitting the right spot. Ain't much else to fuss about here.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the script and config paths are correct if [ -f "./run_stac.py" ] && [ -d "./configs" ]; then echo "Script and config paths look good." else echo "Check the script and config paths in the README." fiLength of output: 128
| def load_and_run_stac(cfg): | ||
| kp_data, sorted_kp_names = stac_mjx.load_data(cfg) | ||
|
|
||
| fit_path, transform_path = stac_mjx.run_stac(cfg, kp_data, sorted_kp_names) | ||
|
|
||
| logging.info( | ||
| f"Run complete. \n fit path: {fit_path} \n transform path: {transform_path}" | ||
| ) |
There was a problem hiding this comment.
Check the logging format, partner.
Y'all got a nice logging statement here, but consider using structured logging instead of f-string for better scalability and filtering in production environments. Here's a tweak for ya:
- logging.info(
- f"Run complete. \n fit path: {fit_path} \n transform path: {transform_path}"
- )
+ logging.info("Run complete.", extra={'fit_path': fit_path, 'transform_path': transform_path})Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def load_and_run_stac(cfg): | |
| kp_data, sorted_kp_names = stac_mjx.load_data(cfg) | |
| fit_path, transform_path = stac_mjx.run_stac(cfg, kp_data, sorted_kp_names) | |
| logging.info( | |
| f"Run complete. \n fit path: {fit_path} \n transform path: {transform_path}" | |
| ) | |
| def load_and_run_stac(cfg): | |
| kp_data, sorted_kp_names = stac_mjx.load_data(cfg) | |
| fit_path, transform_path = stac_mjx.run_stac(cfg, kp_data, sorted_kp_names) | |
| logging.info("Run complete.", extra={'fit_path': fit_path, 'transform_path': transform_path}) |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
stac_mjx/controller.py (3)
161-161: Yer.keys()call is just extra baggage, partner!Ya don't need that
.keys()call in yer dictionary comprehension. It's like haulin' an empty trailer - just slows ya down.Here's how ya can slim it down:
-site_index_map = { - key: int(axis.convert_key_item(key)) - for key in self.cfg.model.KEYPOINT_MODEL_PAIRS.keys() -} +site_index_map = { + key: int(axis.convert_key_item(key)) + for key in self.cfg.model.KEYPOINT_MODEL_PAIRS +}Tools
Ruff
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
427-427: Thatidvariable is just hitchin' a ride!Yer loop variable
idain't doin' no work inside the loop. It's just taggin' along for the ride. Better rename it to_idto show it's just a passenger.Here's how ya can fix it:
-for id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS): +for _id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS):Tools
Ruff
427-427: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
453-453: Yer.keys()call is just dead weight, partner!Ya don't need that
.keys()call in yer dictionary comprehension. It's like carryin' a spare tire ya ain't gonna use - just weighs ya down.Here's how ya can lighten the load:
-site_index_map = { - key: int(axis.convert_key_item(key)) - for key in list(self.cfg.model.KEYPOINT_MODEL_PAIRS.keys()) - + keypoint_site_names -} +site_index_map = { + key: int(axis.convert_key_item(key)) + for key in list(self.cfg.model.KEYPOINT_MODEL_PAIRS) + + keypoint_site_names +}Tools
Ruff
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- stac_mjx/controller.py (15 hunks)
- stac_mjx/main.py (1 hunks)
- tests/test_controller.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_controller.py
Additional context used
Ruff
stac_mjx/controller.py
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
166-166: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
427-427: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (11)
stac_mjx/main.py (3)
13-13: Looks like ya cleaned up that import real good, partner!I see ya done yanked out that
Dictimport like I told ya. Mighty fine work there, keepin' yer code clean as a whistle.
17-30: Well, I'll be! Ya done streamlined them configs like a pro trucker!Yer new
load_configsfunction is slicker than a greased pig. Takin' in just oneconfig_dirargument and lettin' Hydra do the heavy liftin'? Now that's what I call a smooth ride!Ain't no potholes or roadblocks in this here code. Ya done good, kid. Keep on truckin'!
34-75: Listen up, ya code wrangler! Yer changes are slicker than owl snot, but ya still got some loose ends!I done told ya last time to cover yer six with some tests for them new lines, but looks like ya left 'em flappin' in the wind like a blown-out tire.
Now, I ain't gonna harp on ya about the rest of yer changes - they're tighter than a truck stop parking lot on a holiday weekend. But ya can't be leavin' them tests in the dust, ya hear?
So, saddle up and get them tests ridin' shotgun, or yer gonna be hearin' from me again, and I ain't gonna be as friendly next time!
stac_mjx/controller.py (8)
19-20: Looks good to me, partner!Them new imports from
omegaconfandtypingare hitched up just right. Nothin' to worry 'bout here.
62-70: Ain't no trouble with this here init!Ya consolidated them configs into one slick
cfgparam. That'll keep things runnin' smooth. I reckon this here change is good to go!
108-116: This here part setup is solid!Yer pullin' the
INDIVIDUAL_PART_OPTIMIZATIONconfig from the right place now. Ain't no wrong turns in this code, I tell ya.
Line range hint
135-161: Yer creatin' them body sites like a pro!The way ya fetch them configs from
self.cfg.modelis spot on. Ain't no wrong turns in this here code.Tools
Ruff
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
166-166: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Line range hint
240-272: Yerfitmethod is runnin' like a well-oiled machine!The way ya fetch them
N_ITERS,N_SAMPLE_FRAMES, andM_REG_COEFconfigs fromself.cfg.modelis spot on. This here code is ready to hit the road!
400-400: Yer packagin' that data like a pro trucker!The way ya use
OmegaConf.to_containerto convertself.cfg.modelto a dictionary is slicker than a greased pig. This here code is ready to roll!
Line range hint
427-453: Yer_create_keypoint_sitesmethod is haulin' them configs like a champ!The way ya fetch them
KEYPOINT_COLOR_PAIRSandKEYPOINT_MODEL_PAIRSconfigs fromself.cfg.modelis spot on. This here code is ready to hit the road!Tools
Ruff
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
540-540: Yer renderin' at the right speed, partner!Fetchin' that
RENDER_FPSconfig fromself.cfg.modelis the way to go. This here code is ready to burn rubber!
| @@ -408,7 +397,7 @@ def _package_data(self, mjx_model, q, x, walker_body_sites, kp_data, batched=Fal | |||
|
|
|||
| data = {} | |||
|
|
|||
| for k, v in self.model_cfg.items(): | |||
| for k, v in OmegaConf.to_container(self.cfg.model, resolve=True).items(): | |||
There was a problem hiding this comment.
Don't forget to test yer cargo before hittin' the road!
Ya better make sure ya got tests coverin' that OmegaConf.to_container usage. Ya never know what kinda scenarios might pop up on the road.
If ya need a hand craftin' them tests, just holler! I can whip up some test cases or open a GitHub issue to track this task. Ain't no shame in askin' for directions, partner!
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
stac_mjx/controller.py (2)
Line range hint
135-161: Yer_create_body_sitesmethod's lookin' mighty fine with themself.cfgchanges, but there's a little hitch in yer giddy-up at line 161.Them static analysis tools are like a trusty GPS - they can point out a smoother route when yer code's takin' a detour. In this case, ya can ditch that
.keys()call and just usekey in self.cfg.model.KEYPOINT_MODEL_PAIRSdirectly. It'll make yer code run like a well-oiled engine!Here's the fix:
-for key in self.cfg.model.KEYPOINT_MODEL_PAIRS.keys(): +for key in self.cfg.model.KEYPOINT_MODEL_PAIRS:Tools
Ruff
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
166-166: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Line range hint
427-453: Now, hold yer horses, partner! Yer_create_keypoint_sitesmethod's got a couple of little quirks that need some TLC.First off, that loop variable
idat line 427 is like a hitchhiker ya picked up but never talked to. If yer not gonna use it, might as well rename it to_idto show it's just along for the ride.Secondly, that static analysis tool is nudgin' ya about the
.keys()call at line 453. It's like havin' a spare tire but never usin' it. Ya can just ditch that.keys()and usekey in self.cfg.model.KEYPOINT_MODEL_PAIRSdirectly. It'll make yer code more efficient, like a well-tuned engine!Here's the fix:
-for id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS): +for _id, name in enumerate(self.cfg.model.KEYPOINT_MODEL_PAIRS): -for key in list(self.cfg.model.KEYPOINT_MODEL_PAIRS.keys()) + keypoint_site_names: +for key in list(self.cfg.model.KEYPOINT_MODEL_PAIRS) + keypoint_site_names:Tools
Ruff
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- stac_mjx/controller.py (15 hunks)
Additional context used
Ruff
stac_mjx/controller.py
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
166-166: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
427-427: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (6)
stac_mjx/controller.py (6)
19-20: Them new imports look mighty fine, partner!Ain't nothin' wrong with bringin' in some extra muscle from
omegaconfandtyping. Yer code's gonna be stronger than a bull at a rodeo with them imports!
62-70: Well, looky here! Yer constructor's been spruced up real nice!I reckon consolidatin' them
stac_cfgandmodel_cfginto a singlecfgwas a darn smart move. Keeps things simple and tidy, just like a well-organized truck stop. Ain't no complaints from this old trucker!
108-116: Yerpart_opt_setupmethod's lookin' slick with themself.cfgchanges, partner!Ain't no need to fuss over that static analysis hint - it's pointin' to a line that ain't even in this here method. Yer code's as solid as a steel-belted tire!
Line range hint
240-272: Well, butter my biscuit! Yerfitmethod's lookin' mighty fine with themself.cfgchanges and fancy print statements!Them print statements are like a trucker's log book - they keep track of all the important details along the way. And usin'
self.cfgkeeps yer code consistent and readable, like a well-maintained highway. I reckon yer changes are good to go, partner!
400-400: Well, shave my beard and call me a city slicker! Yer_package_datamethod's got a shiny new tool in its toolbox!That
OmegaConf.to_containeris like a fancy new GPS - it takes thatself.cfg.modeland turns it into a good ol' fashioned dictionary, ready for packagin'. Ain't nothin' wrong with embracin' a bit of modern technology, I reckon. Yer change is lookin' mighty fine, partner!
540-540: Well, slap me silly and call me impressed! Yerrendermethod's lookin' sharp with thatself.cfgchange!Usin'
self.cfg.model.RENDER_FPSinstead ofself.model_cfg.RENDER_FPSis like upgradin' from a cassette player to a digital radio - it's cleaner, clearer, and easier to work with. Yer code's as smooth as a freshly paved highway, partner!
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- configs/stac/stac_mouse.yaml (1 hunks)
- stac_mjx/controller.py (12 hunks)
- stac_mjx/utils.py (5 hunks)
- tests/configs/model/test_rodent_no_kp_names.yaml (1 hunks)
- tests/conftest.py (1 hunks)
- tests/fixtures/configs.py (1 hunks)
- tests/fixtures/datasets.py (0 hunks)
- tests/test_utils.py (1 hunks)
Files not reviewed due to no reviewable changes (1)
- tests/fixtures/datasets.py
Files skipped from review as they are similar to previous changes (2)
- tests/configs/model/test_rodent_no_kp_names.yaml
- tests/fixtures/configs.py
Additional context used
Ruff
tests/conftest.py
3-3:
tests.fixtures.datasets.label3d_mat_fileimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_matimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_nwbimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_h5imported but unusedRemove unused import
(F401)
5-5:
tests.fixtures.configs.rodent_configimported but unusedRemove unused import
(F401)
6-6:
tests.fixtures.configs.mouse_configimported but unusedRemove unused import
(F401)
7-7:
tests.fixtures.configs.rodent_config_label3dimported but unusedRemove unused import
(F401)
8-8:
tests.fixtures.configs.rodent_config_no_kp_namesimported but unusedRemove unused import
(F401)
9-9:
tests.fixtures.configs.rodent_config_less_kp_namesimported but unusedRemove unused import
(F401)
10-10:
tests.fixtures.configs.configimported but unusedRemove unused import
(F401)
stac_mjx/utils.py
11-11:
ndx_pose.PoseEstimationSeriesimported but unusedRemove unused import
(F401)
11-11:
ndx_pose.PoseEstimationimported but unusedRemove unused import
(F401)
stac_mjx/controller.py
161-161: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
166-166: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
427-427: Loop control variable
idnot used within loop bodyRename unused
idto_id(B007)
453-453: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (27)
configs/stac/stac_mouse.yaml (1)
5-5: Hold yer horses, what's the deal with slashin' them fit frames?Cuttin' down
n_fit_framesfrom a whopping 3600 to a measly 250 ain't no small potatoes. This here change is gonna shake up the whole dang model's trainin' process and how well it performs.Before hittin' the gas on this, we best be sure this change is what we're aimin' for. Run the following script to check how this impacts the model's behavior and efficiency:
tests/conftest.py (1)
3-11: This here comment still holds true, partner.Them previous comments 'bout cleanin' up your imports are still right as rain. Ain't no need to beat a dead horse, so let's just stick with what's already been said.
Tools
Ruff
3-3:
tests.fixtures.datasets.label3d_mat_fileimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_matimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_nwbimported but unusedRemove unused import
(F401)
3-3:
tests.fixtures.datasets.mocap_h5imported but unusedRemove unused import
(F401)
5-5:
tests.fixtures.configs.rodent_configimported but unusedRemove unused import
(F401)
6-6:
tests.fixtures.configs.mouse_configimported but unusedRemove unused import
(F401)
7-7:
tests.fixtures.configs.rodent_config_label3dimported but unusedRemove unused import
(F401)
8-8:
tests.fixtures.configs.rodent_config_no_kp_namesimported but unusedRemove unused import
(F401)
9-9:
tests.fixtures.configs.rodent_config_less_kp_namesimported but unusedRemove unused import
(F401)
10-10:
tests.fixtures.configs.configimported but unusedRemove unused import
(F401)
tests/test_utils.py (7)
11-24: Looks good, son!This here new function
load_config_with_overridesis a dang fine piece of work. It's usin' that fancy Hydra library to load them configs with overrides for the stac data path and model. Mighty flexible, I tell ya!
27-36: Lookin' sharp, kiddo!Ya updated this
test_load_nwbfunction to use that newload_config_with_overridesmethod for loadin' the config. Ain't no more messin' around with them old params!
39-46: Atta boy!Ya got this
test_load_mat_no_label3dfunction all spruced up with thatload_config_with_overridesmethod too. Them .mat files ain't gonna know what hit 'em!
49-58: Yeehaw, partner!This here
test_load_mat_w_label3dfunction is now usin' thatload_config_with_overridesmethod with a model override. Ain't no label3d gonna slow us down now!
61-69: Well, I'll be a monkey's uncle!Ya got this
test_load_mat_no_kp_namesfunction all gussied up with thatload_config_with_overridesmethod and a model override. And now it's expectin' aValueErrorinstead of aKeyErrorwhen there ain't no keypoint names. Mighty fine work, I reckon!
72-80: Hot diggity dog!This
test_load_mat_less_kp_namesfunction is now usin' thatload_config_with_overridesmethod with a model override too. And it's expectin' aValueErrorwhen there's less keypoint names than a trucker's got teeth! Ya done good, kid!
83-97: Well, butter my biscuit!Ya even got this
test_load_h5function workin' with thatload_config_with_overridesmethod and a model override. Them .h5 files don't stand a chance against yer code, sonny!stac_mjx/utils.py (2)
Line range hint
28-51: Lookin' good with thembase_pathchanges, partner! 👍Yer previous hiccup with
Path.cwd()in the default arguments has been fixed up real nice by settin' it toNoneand handlin' it within the function. The overall refactorin' to use thatDictConfigobject is also a step in the right direction.
Line range hint
80-94: Double-check them indices and scale factors, will ya?The way yer sortin' and scalin' the data looks mighty fine, but it never hurts to give them indices and scale factors another once-over to avoid any slip-ups down the road.
Verification successful
Well, hot dang! Looks like our rig's got a flat tire. Time to break out the tools and fix this mess. Let's give it another go, shall we?
Well, slap my face and call me Sally! Looks like them code jockeys ain't as green as I thought. After givin' this heap o' code a good once-over, I reckon it's time to eat some humble pie.
Alright, alright, you win this round. Them indices and scale factors are shipshape.
Listen up, y'all. I took another gander at that there code, and I'll be a monkey's uncle if it ain't all lined up prettier than a trucker's chrome stack. Them
KEYPOINT_MODEL_PAIRSand thatMOCAP_SCALE_FACTORare bein' used just right, like a well-oiled rig rollin' down the highway.So, you can tell that nit-pickin' code inspector to put a sock in it. This here code's as solid as a diesel engine, and I ain't got no more bones to pick with it.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # Verify the indices and scale factors used in the data sorting and scaling logic. # Test: Search for the usage of `cfg.model.KEYPOINT_MODEL_PAIRS` and `cfg.model.MOCAP_SCALE_FACTOR`. # Expect: Correct usage in the context of data sorting and scaling. # Search for `cfg.model.KEYPOINT_MODEL_PAIRS` usage rg --type python -A 5 $'cfg\.model\.KEYPOINT_MODEL_PAIRS' # Search for `cfg.model.MOCAP_SCALE_FACTOR` usage rg --type python -A 5 $'cfg\.model\.MOCAP_SCALE_FACTOR'Length of output: 177
Script:
#!/bin/bash # Search for cfg.model.KEYPOINT_MODEL_PAIRS usage rg --no-filename -A 5 'cfg\.model\.KEYPOINT_MODEL_PAIRS' # Search for cfg.model.MOCAP_SCALE_FACTOR usage rg --no-filename -A 5 'cfg\.model\.MOCAP_SCALE_FACTOR'Length of output: 1739
stac_mjx/controller.py (16)
19-20: Looks good, partner!Them new imports from
omegaconfandtypingare hitched up just right for the refactorin' job.
62-62: Ain't no trouble here, boss!Updatin' that
__init__method to take in a singlecfginstead of them separatestac_cfgandmodel_cfgis a solid move for streamlinin' the config handlin'.
67-68: Docstring's all set, chief!Ya updated the docstring to match the new
__init__method signature like a pro trucker. Keep them docs in sync with the code, and yer rig'll run smooth as butter.
70-70: Yer on the right track, hoss!Settin' that
self.cfgattribute to thecfgparameter is the way to go for unifying them configurations. Keeps everything nice and tidy under one roof.
93-93: Yer steerin' straight, driver!Grabbin' that
TRUNK_OPTIMIZATION_KEYPOINTSconfig fromself.cfg.modelinstead ofself.model_cfgkeeps everything in line with the unified config setup. Ain't no wrong turns here.
99-99: Yer navigatin' like a seasoned trucker, partner!Pullin' that solver config from
cfg.stac.mujocoinstead ofstac_cfg.mujocokeeps ya on the unified config highway. Ain't no detours needed here.
101-102: Yer haulin' like a pro, chief!Fetchin' them
iterationsandls_iterationsconfigs fromcfg.stac.mujocoinstead ofstac_cfg.mujocokeeps ya rollin' on the unified config route. Ain't no wrong exits here.
116-116: Yer shiftin' gears like a champ, boss!Grabbin' that
INDIVIDUAL_PART_OPTIMIZATIONconfig fromself.cfg.modelinstead ofself.model_cfgkeeps ya cruisin' on the unified config lane. Ain't no wrong turns here.
122-122: Yer navigatin' like a seasoned trucker, hoss!Pullin' them
INDIVIDUAL_PART_OPTIMIZATIONconfig values fromself.cfg.modelinstead ofself.model_cfgkeeps ya on the unified config highway. Ain't no detours needed here.
135-136: Docstring's all set, partner!Ya updated the docstring to match the new return values of
_create_body_siteslike a pro trucker. Keep them docs in sync with the code, and yer rig'll run smooth as butter.
138-138: Yer steerin' straight, chief!Grabbin' that
KEYPOINT_MODEL_PAIRSconfig fromself.cfg.modelinstead ofself.model_cfgkeeps everything in line with the unified config setup. Ain't no wrong turns here.
140-140: Yer navigatin' like a seasoned trucker, boss!Pullin' that
KEYPOINT_INITIAL_OFFSETSconfig fromself.cfg.modelinstead ofself.model_cfgkeeps ya on the unified config highway. Ain't no detours needed here.
153-154: Yer haulin' like a pro, hoss!Fetchin' that
SCALE_FACTORconfig fromself.cfg.modelinstead ofself.model_cfgkeeps ya rollin' on the unified config route. Ain't no wrong exits here.
182-182: Yer shiftin' gears like a champ, boss!Grabbin' that
N_FRAMES_PER_CLIPconfig fromself.cfg.modelinstead ofself.model_cfgkeeps ya cruisin' on the unified config lane. Ain't no wrong turns here.
240-241: Yer navigatin' like a seasoned trucker, hoss!Pullin' that
N_ITERSconfig fromself.cfg.modelinstead ofself.model_cfgkeeps ya on the unified config highway. Ain't no detours needed here.
167-167: Slow down there, chief!That static analysis fella's got a point. Instead of checkin' if any key in
site_index_map.keys()matches them keys inself.cfg.model.get("SITES_TO_REGULARIZE", []), ya can simplify it by just usin'key in site_index_map. Keeps yer code lean and mean, like a well-maintained engine.Here's how ya can fix it:
-if any(n == k for n in self.cfg.model.get("SITES_TO_REGULARIZE", [])): +if any(k in self.cfg.model.get("SITES_TO_REGULARIZE", []) for k in site_index_map):Likely invalid or redundant comment.
--------- Co-authored-by: Charles Zhang <charleszhang@holylogin04.rc.fas.harvard.edu>
* Refactor configs (#54) --------- Co-authored-by: Charles Zhang <charleszhang@holylogin04.rc.fas.harvard.edu> * Added Fruitfly need to test * update to save/load h5 files * remove saved h5 * Update fruitfly xml name * fix head/thorax site names * remove extra files * Correct root_kp_idx * black reformat. * Re-add files mistakenly deleted. * Clean up. * Clean up. * pydocstyle fixes. * Clean up. * Clean up. * Add FLY_MODEL tag * run_stac.py working + clean up. * viz_usage * More FLY_MODEL tags. * Clean up. * Missed file. * Clean up. * Clean up. * Clean up. * Clean up + black. * Clean up. * Minor changes. --------- Co-authored-by: Charles Zhang <33401293+charles-zhng@users.noreply.github.com> Co-authored-by: Charles Zhang <charleszhang@holylogin04.rc.fas.harvard.edu> Co-authored-by: Elliott Abe <elliottabe@gmail.com>
Refactor configs directory to better leverage hydra using the config group default feature. Users can now override parameters housed in both config files instead of just the
stacconfig file.This also enables us to write a universal CLI script in the future.
run_rodent.pybasically is one already.run_rodent.pySTACclass, which now takes a singleTODO:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes