Skip to content

Comments

fix: correct misleading comments and docs in BinaryJointAction#4679

Open
alltheseas wants to merge 2 commits intoisaac-sim:mainfrom
alltheseas:fix/binary-joint-action-comment-convention
Open

fix: correct misleading comments and docs in BinaryJointAction#4679
alltheseas wants to merge 2 commits intoisaac-sim:mainfrom
alltheseas:fix/binary-joint-action-comment-convention

Conversation

@alltheseas
Copy link

Summary

Fix misleading inline comments and an imprecise docstring in BinaryJointAction.process_actions.

  • The inline comments stated # true: close, false: open, which describes the binary_mask variable but reads as describing the action convention — the opposite of actual behavior
  • The class docstring said "positive values" for float open actions, but the implementation uses < 0, so zero maps to open. Updated to "non-negative values"

No behavior change. Comments and docs now match the code.

Fixes #4670

Changes

  • source/isaaclab/isaaclab/envs/mdp/actions/binary_joint_actions.py
    • Class docstring: "positive" → "non-negative" for float open actions
    • Inline comments: clarify action-to-command mapping unambiguously

Signed-off-by: alltheseas alltheseas@users.noreply.github.com

The docstring for `resolve_matching_names` incorrectly described the
behavior of the `preserve_order` parameter — the descriptions for
True and False were swapped.

Fixes isaac-sim#4493

Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
The inline comments in process_actions stated "true: close, false:
open" which describes the binary_mask variable but reads as describing
the action convention, contradicting actual behavior.

Also fix class docstring: "positive" -> "non-negative" for float
actions, since zero maps to open (the mask uses < 0, not <= 0).

Fixes isaac-sim#4670

Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
@alltheseas alltheseas requested a review from ooctipus as a code owner February 21, 2026 07:24
@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Feb 21, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 21, 2026

Greptile Summary

This PR fixes misleading documentation in two areas:

  1. BinaryJointAction: Updated class docstring from "positive values" to "non-negative values" for float open actions, matching the actual implementation (action >= 0 maps to open). Clarified inline comments to describe action-to-command mapping from the user's perspective rather than the internal binary_mask variable.

  2. resolve_matching_names: Fixed inverted docstring that incorrectly described the preserve_order parameter behavior - the documentation now correctly states that preserve_order=False (default) follows list order, while preserve_order=True follows regex key order.

Both changes are documentation-only with no behavioral impact. The code logic remains unchanged and functions correctly.

Confidence Score: 5/5

  • This PR is completely safe to merge with zero risk
  • Documentation-only changes with no code behavior modifications - the updates correct misleading comments and docstrings to accurately match existing implementation logic
  • No files require special attention

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/envs/mdp/actions/binary_joint_actions.py Corrected misleading docstring and inline comments to accurately describe action-to-command mapping behavior
source/isaaclab/isaaclab/utils/string.py Fixed inverted docstring describing preserve_order parameter behavior with clearer examples

Last reviewed commit: 3e0f702

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug Report] BinaryJointAction.process_actions: bool-action convention mismatch / confusing comment (True treated as open, False as close)

1 participant