render new marker offsets, add option to visualize marker error #63
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, focusing on performance enhancements, code readability improvements, and adjustments to rendering and optimization processes. Key changes include an increase in processing speed in a Jupyter notebook, the addition of a new return value in a function, and updates to rendering methods to incorporate new parameters and improve visual output. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 #63 +/- ##
==========================================
- Coverage 42.56% 41.77% -0.79%
==========================================
Files 9 9
Lines 531 541 +10
==========================================
Hits 226 226
- Misses 305 315 +10 ☔ 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: 2
Outside diff range and nitpick comments (4)
stac_mjx/viz.py (1)
21-21: LGTM! Consider adding a type hint for consistency.The addition of the
show_marker_errorparameter aligns well with the PR objective to add an option for visualizing marker error. The default value ofFalseensures backward compatibility.For consistency with other parameters, consider adding a type hint:
- show_marker_error=False, + show_marker_error: bool = False,demos/viz_usage.ipynb (1)
108-114: Consider removing empty code cells.Three empty code cells have been added at the end of the notebook. While these might be intended as placeholders for future code, they currently don't serve any purpose and may unnecessarily clutter the notebook. Consider removing these empty cells if you don't have immediate plans to use them.
stac_mjx/controller.py (2)
262-262: Add tests forcompute_stac.offset_optimizationThe line where
self._offsetsis updated is critical, but it's not covered by tests. It's important to ensure that this function behaves as expected, especially since it affects the offsets used throughout the model.Would you like assistance in creating unit tests for this function or should we open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 262-262: stac_mjx/controller.py#L262
Added line #L262 was not covered by tests
530-543: Visualize marker errors with tendons whenshow_marker_erroris TrueThe conditional block adds tendons between the keypoint sites and the new marker sites to visualize the errors. However, this new functionality is not covered by tests.
Would you like help in writing tests to ensure this visualization feature works correctly or should we open a GitHub issue to track this task?
Tools
Ruff
533-533: Local variable
rgbais assigned to but never usedRemove assignment to unused variable
rgba(F841)
GitHub Check: codecov/patch
[warning] 530-531: stac_mjx/controller.py#L530-L531
Added lines #L530 - L531 were not covered by tests
[warning] 533-534: stac_mjx/controller.py#L533-L534
Added lines #L533 - L534 were not covered by tests
[warning] 541-542: stac_mjx/controller.py#L541-L542
Added lines #L541 - L542 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- demos/viz_usage.ipynb (3 hunks)
- stac_mjx/compute_stac.py (1 hunks)
- stac_mjx/controller.py (7 hunks)
- stac_mjx/viz.py (3 hunks)
Additional context used
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)
GitHub Check: codecov/patch
stac_mjx/compute_stac.py
[warning] 175-175: stac_mjx/compute_stac.py#L175
Added line #L175 was not covered by testsstac_mjx/controller.py
[warning] 262-262: stac_mjx/controller.py#L262
Added line #L262 was not covered by tests
[warning] 393-393: stac_mjx/controller.py#L393
Added line #L393 was not covered by tests
[warning] 515-515: stac_mjx/controller.py#L515
Added line #L515 was not covered by tests
[warning] 518-519: stac_mjx/controller.py#L518-L519
Added lines #L518 - L519 were not covered by tests
[warning] 530-531: stac_mjx/controller.py#L530-L531
Added lines #L530 - L531 were not covered by tests
[warning] 533-534: stac_mjx/controller.py#L533-L534
Added lines #L533 - L534 were not covered by tests
[warning] 541-542: stac_mjx/controller.py#L541-L542
Added lines #L541 - L542 were not covered by tests
[warning] 544-545: stac_mjx/controller.py#L544-L545
Added lines #L544 - L545 were not covered by tests
[warning] 548-548: stac_mjx/controller.py#L548
Added line #L548 was not covered by tests
[warning] 553-553: stac_mjx/controller.py#L553
Added line #L553 was not covered by tests
[warning] 555-555: stac_mjx/controller.py#L555
Added line #L555 was not covered by tests
[warning] 557-560: stac_mjx/controller.py#L557-L560
Added lines #L557 - L560 were not covered by tests
[warning] 582-582: stac_mjx/controller.py#L582
Added line #L582 was not covered by testsstac_mjx/viz.py
[warning] 49-49: stac_mjx/viz.py#L49
Added line #L49 was not covered by tests
Ruff
stac_mjx/controller.py
533-533: Local variable
rgbais assigned to but never usedRemove assignment to unused variable
rgba(F841)
Additional comments not posted (9)
stac_mjx/viz.py (1)
65-65: LGTM! Verify render method implementation.The
show_marker_errorparameter is correctly passed to thestac.render()method, implementing the new feature as described in the PR objectives.To ensure complete implementation:
- Verify that the
rendermethod in theSTACclass has been updated to handle this new parameter.- Check if appropriate tests have been added for this new functionality.
Run the following script to locate the
STACclass definition and itsrendermethod:#!/bin/bash # Description: Locate STAC class and render method # Find the file containing the STAC class definition stac_file=$(rg -l 'class STAC' --type python) # If found, search for the render method within the STAC class if [ -n "$stac_file" ]; then echo "STAC class found in: $stac_file" echo "Render method definition:" ast-grep --lang python --pattern $'class STAC { $$$ render($$$) { $$$ } $$$ }' "$stac_file" else echo "STAC class not found" fistac_mjx/compute_stac.py (1)
175-175:⚠️ Potential issueApprove changes and suggest improvements
The addition of
offset_opt_paramto the return statement is a good improvement. It aligns with the PR objectives of rendering new marker offsets and potentially visualizing marker error.However, there are a few points to consider:
- The function's docstring should be updated to reflect the new return value.
- Unit tests should be added or updated to cover this new return value, as indicated by the static analysis tool.
To address these points, please:
- Update the docstring to include
offset_opt_paramin the Returns section.- Add or update unit tests to cover the new return value.
To ensure the new return value is properly utilized, please run the following script:
This will help us confirm that the new return value is being used appropriately throughout the codebase.
Verification successful
Verified usage of
offset_opt_paramThe new return value
offset_opt_paramis properly utilized in the codebase:
- In
stac_mjx/controller.py,offset_opt_paramis assigned toself._offsets.- In
stac_mjx/compute_stac.py,offset_opt_paramis assigned fromres.params.No issues found with the current usage.
However, please ensure that:
- The function’s docstring in
stac_mjx/compute_stac.pyis updated to includeoffset_opt_paramin the Returns section.- Unit tests are added or updated to cover the new return value.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of offset_opt_param in other parts of the codebase # Search for function calls to offset_optimization echo "Searching for offset_optimization function calls:" rg --type python 'offset_optimization\(' -A 5 # Search for variable assignments that might capture the new return value echo "Searching for potential assignments of the new return value:" rg --type python 'offset_optimization\(.*\)' -A 2Length of output: 401
Script:
#!/bin/bash # Description: Verify the usage of offset_opt_param in other parts of the codebase # Search for function calls to offset_optimization in Python files echo "Searching for offset_optimization function calls in Python files:" rg --glob "*.py" 'offset_optimization\(' -A 5 # Search for variable assignments that capture the new return value in Python files echo "Searching for potential assignments of the new return value in Python files:" rg --glob "*.py" 'offset_opt_param\s*=' -A 2Length of output: 1180
Tools
GitHub Check: codecov/patch
[warning] 175-175: stac_mjx/compute_stac.py#L175
Added line #L175 was not covered by testsdemos/viz_usage.ipynb (2)
88-95: Improved readability with multi-line function call.The reformatting of the
stac_mjx.viz_stacfunction call enhances code readability by clearly presenting each parameter on a separate line. This is a good practice, especially for functions with multiple parameters, as it makes the code easier to read and maintain.
69-69: False positive: No AWS credentials in encoded video data.A static analysis tool flagged this line as potentially containing AWS credentials. However, this is a false positive. The flagged content is part of a base64 encoded video source in the HTML output of the notebook, not actual code. It does not contain any real AWS credentials or sensitive information.
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)
stac_mjx/controller.py (5)
146-146: Change sphere site opacity for better visualizationThe
rgbavalue for the sphere site has been changed to"0 0 0 0.8", making it slightly transparent. This enhancement can improve the visualization by allowing other objects behind the sphere to be partially visible.
393-395: Confirm consistency when retrieving offsets in_package_dataOffsets are now retrieved directly from
self._offsets. Ensure thatself._offsetsis synchronized with any updates to the model's site positions to prevent discrepancies.Tools
GitHub Check: codecov/patch
[warning] 393-393: stac_mjx/controller.py#L393
Added line #L393 was not covered by tests
473-473: Introduceshow_marker_errorparameter inrendermethodThe new optional parameter
show_marker_error: bool = Falseallows users to visualize the error between markers and keypoints by rendering a red line. This is a valuable feature for debugging and improving model accuracy.
513-528: Add new marker sites for offset positionsThe code adds new marker sites with names suffixed by
_new, representing the new offset positions. This is necessary for visualizing the updated marker positions.Tools
GitHub Check: codecov/patch
[warning] 515-515: stac_mjx/controller.py#L515
Added line #L515 was not covered by tests
[warning] 518-519: stac_mjx/controller.py#L518-L519
Added lines #L518 - L519 were not covered by tests
582-582: Verify the use ofmj_fwdPositionin renderingThe call to
mujoco.mj_fwdPositionupdates the model's positions without computing velocities or accelerations. Confirm that this is sufficient for rendering purposes and that dynamic simulation is not required here.Tools
GitHub Check: codecov/patch
[warning] 582-582: stac_mjx/controller.py#L582
Added line #L582 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
demos/viz_usage.ipynb (1)
108-114: Consider removing the empty code cell.An empty code cell has been added at the end of the notebook. If this cell is not intended for immediate use, consider removing it to keep the notebook concise and focused.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
demos/demo_viz.pis excluded by!**/*.p
Files selected for processing (1)
- demos/viz_usage.ipynb (3 hunks)
Additional context used
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 (4)
demos/viz_usage.ipynb (4)
62-62: Significant performance improvement observed.The progress indicator shows a substantial increase in processing speed from 4.36it/s to 18.97it/s. This aligns with the mentioned performance improvements in the summary.
69-69: Updated video content detected.The base64 encoded video source has been changed, which likely represents updated video content being displayed in the notebook.
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)
83-95: Improved code formatting for better readability.The
stac_mjx.viz_stacfunction call has been reformatted with each parameter on a separate line. This change enhances code readability without altering the functionality.
69-69: Potential security concern: Verify the content of the base64 encoded string.The static analysis tool has flagged this line as potentially containing AWS credentials. While this is likely a false positive due to the nature of base64 encoded video data, it's crucial to verify that no sensitive information is inadvertently included in the encoded string.
To ensure no sensitive data is present, you can decode a small portion of the base64 string and check its contents. Here's a script to do that:
This script will output the hexdump of the first part of the decoded base64 string. Verify that it starts with typical MP4 file headers and doesn't contain any readable text that could be sensitive information.
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)
| @@ -45,7 +46,7 @@ def viz_stac( | |||
| d = pickle.load(file) | |||
| qposes = np.array(d["qpos"]) | |||
| kp_data = np.array(d["kp_data"]) | |||
| kp_names = d["kp_names"] | |||
| kp_names = d["KP_NAMES"] | |||
| @@ -538,10 +571,11 @@ def render( | |||
| # render while stepping using mujoco | |||
| with imageio.get_writer(save_path, fps=self.cfg.model.RENDER_FPS) as video: | |||
| for qpos, kps in tqdm(zip(qposes, kp_data)): | |||
| # Set keypoints | |||
| # Set keypoints--they're in cartesian space, but since they're attached to the worldbody they're the same as offsets | |||
There was a problem hiding this comment.
Eh, what do you mean by "the same as offsets" in this context?
marker positions werent properly rendered before (still have not confirmed why), but a working solution is to create new marker sites instead of updating the relative positions via learned offsets as we were doing previously.
Also added the option in
viz_stacto render a red line showing the error btwn markers and keypoints.Summary by CodeRabbit
New Features
Bug Fixes
Documentation