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
Suggested reviewers
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 43.55% 42.75% -0.80%
==========================================
Files 9 9
Lines 535 545 +10
==========================================
Hits 233 233
- Misses 302 312 +10 ☔ View full report in Codecov by Sentry. |
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)
stac_mjx/controller.py
Outdated
| @@ -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?
There was a problem hiding this comment.
The keypoints represented in global cartesian space can be considered offsets relative to the worldbody. I noted this because setting site positions is always relative to the parent body, so one might get confused about why this works with the keypoints' global frame representation. (The keypoint sites are created with the worldbody as the parent)
There was a problem hiding this comment.
Would it be more clear to say "their the same as global (world) coordinates"? Offsets seems relative to me.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
stac_mjx/viz.py (1)
65-65: LGTM: Implementation change is correct, consider adding documentationThe new
show_marker_errorparameter is correctly passed to thestac.rendermethod. This change is minimal and aligns with the function's role as a wrapper.Consider updating the function's docstring to include information about the new
show_marker_errorparameter and its purpose.stac_mjx/compute_stac.py (1)
179-179: Approve the change and update documentationThe modification to return
offset_opt_paramis a good addition as it provides direct access to the optimized offset parameters. This change aligns with the PR objectives of rendering new marker offsets.To maintain code clarity and prevent potential issues:
- Update the function's docstring to reflect the new return value.
- Ensure that any code calling this function is updated to handle the new three-element tuple return value.
Example docstring update:
Returns: Tuple[mjx.Model, mjx.Data, jp.ndarray]: An updated MJX Model, MJX Data, and the optimized offset parametersstac_mjx/stac.py (1)
146-146: Adjust RGBA Transparency ValueThe RGBA value for the site has been changed to
"0 0 0 0.8", which adjusts the alpha transparency to0.8. Ensure that this level of transparency achieves the desired visual effect and is consistent with other visual elements in the model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- stac_mjx/compute_stac.py (1 hunks)
- stac_mjx/stac.py (7 hunks)
- stac_mjx/viz.py (2 hunks)
🔇 Additional comments (10)
stac_mjx/viz.py (2)
21-21: LGTM: New parameter aligns with PR objectivesThe addition of the
show_marker_errorparameter with a default value ofFalseis well-implemented. It provides the new functionality described in the PR objectives while maintaining backward compatibility.
Line range hint
49-49: Verification:kp_namesusage is consistentAddressing a past review comment: The current code uses
d["kp_names"], which is consistent with the usage in other parts of the codebase. The previously mentioned change tod["KP_NAMES"]is not present in this version, resolving the potential inconsistency.To ensure consistency across the codebase, run the following script:
This will help confirm that
kp_namesis used consistently throughout the project.✅ Verification successful
Verification:
kp_namesusage is consistentAfter reviewing the usage of
kp_namesacross the codebase, it is confirmed thatkp_namesis used consistently instac_mjx/viz.pyand other relevant files. The earlier concern about changingd["kp_names"]tod["KP_NAMES"]does not apply, as all references usekp_namesuniformly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of 'kp_names' across the codebase # Search for 'kp_names' in Python files rg -i 'kp_names' --glob '*.py'Length of output: 4293
stac_mjx/compute_stac.py (1)
Line range hint
1-279: Verify implementation of marker error visualizationThe PR objectives mention adding an option to visualize marker error. However, there are no visible changes in this file related to this feature.
Please confirm if the visualization feature is implemented in another file or if it's missing from this PR. If it's in another file, consider updating the PR description to clarify where this feature is implemented.
To verify the implementation, you can run the following script:
This script will help identify any new or modified visualization-related code in other files of the repository.
stac_mjx/stac.py (7)
485-485: Update Docstring to Includeshow_marker_errorParameterThe docstring for the
rendermethod now includes the new parametershow_marker_error, accurately describing its purpose and default value. This ensures that the method's documentation is up-to-date.
528-539: Implement Conditional Tendons for Error VisualizationWhen
show_marker_errorisTrue, tendons are added to visualize the error between markers and keypoints:if show_marker_error: for key, v in self.cfg.model.KEYPOINT_MODEL_PAIRS.items(): tendon = self._root.tendon.add( "spatial", name=key + "-" + v, width="0.001", rgba="255 0 0 1", # Red limited=False, ) tendon.add("site", site=key + "_kp") tendon.add("site", site=key + "_new")Verify that the tendons are correctly defined and that they render appropriately without introducing unintended side effects.
471-471: Update Method Signature to Includeshow_marker_errorThe
rendermethod now includes a new parameter:def render(..., show_marker_error: bool = False):Ensure that all calls to
renderin the codebase are updated accordingly, and the default behavior remains unaffected for existing usages.Run the following script to identify all calls to
render:#!/bin/bash # Description: Find all calls to the 'render' method outside its definition. # Expected: Review each call to ensure it handles the new parameter if necessary. rg --type py --pcre2 '(?<!def )render\(' stac_mjx/
511-526: Add New Marker Sites for Offsets in RenderingThe code adds new marker sites for offsets to visualize the adjusted marker positions:
for (key, v), pos in zip(self.cfg.model.KEYPOINT_MODEL_PAIRS.items(), offsets.reshape((-1, 3))): parent = self._root.find("body", v) parent.add( "site", name=key + "_new", type="sphere", size=[0.005], rgba="0 0 0 1", pos=pos, group=2, )Ensure that these new sites are correctly integrated into the model and that their names do not conflict with existing site names.
Run the following script to check for naming conflicts with site names:
#!/bin/bash # Description: Check for duplicate site names in the model XML files. # Expected: No duplicate site names should be found. rg --type xml '<site name="' stac_mjx/ | gawk -F'"' '{print $2}' | sort | uniq -d
578-578: Update Function Call tomujoco.mj_fwdPositionThe function call has been changed from
mujoco.mj_forwardtomujoco.mj_fwdPosition:mujoco.mj_fwdPosition(render_mj_model, mj_data)This computes only the position-related quantities, which can be more efficient when full dynamics are not required. Confirm that no downstream code relies on computations performed by
mj_forwardthat are not covered bymj_fwdPosition.Run the following script to find any remaining uses of
mujoco.mj_forward:#!/bin/bash # Description: Find all occurrences of 'mujoco.mj_forward' in the codebase. # Expected: Ensure that 'mujoco.mj_forward' is only used where full dynamics are necessary. rg --type py 'mujoco\.mj_forward' stac_mjx/
389-393:⚠️ Potential issueEnsure Consistent Assignment of
offsetsin_package_dataIn the
_package_datamethod,offsetsis assigned differently based on thebatchedflag:if batched: offsets = get_batch_offsets(mjx_model, self._body_site_idxs)[0] else: offsets = self._offsetsVerify that
self._offsetsis correctly initialized before being used here and that the batched and non-batched cases handleoffsetsconsistently.Run the following script to check initialization and usage of
self._offsets:
261-261:⚠️ Potential issueVerify Usage of
self._offsetsAfter AssignmentYou've updated the assignment to include
self._offsets:mjx_model, mjx_data, self._offsets = compute_stac.offset_optimization(...)Ensure that
self._offsetsis properly utilized throughout the class after this change, especially in methods that depend on the updated offsets.Run the following script to verify all usages of
self._offsets:✅ Verification successful
Usage of
self._offsetsAfter Assignment VerifiedAll references to
self._offsetsinstac_mjx/stac.pyhave been confirmed to utilize the updated offsets appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of 'self._offsets' within the 'stac_mjx' directory. # Expected: All references to 'self._offsets' reflect the updated usage. rg --type py 'self\._offsets' stac_mjx/Length of output: 442
|
Still looking into the KP_NAMES vs kp_names thing |
seems like kp_names is correct. merging now |
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