Repository navigation
[Core] Fix image normals data type resolution - #8375
Conversation
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
All reviewed code categories look good.
- PR description: The description matches the signature fix, resulting manager behavior, three-file scope, and reported validation; no performance evidence is needed for this construction-time parameter change.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: Style consistency looks good.
- Test quality: Test quality looks good.
No blocking issues. No inline findings.
Automated review; human maintainers own approval decisions.
 |
|
@StafaH not sure if this is the right fix, pls let me know |
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (Claude Code). Please double-check before acting on it.
The fix is correct. ManagerTermBase.__init__ fills in keyword-only defaults through _populate_term_defaults (manager_base.py:465-468), so the new test goes through the same path the observation manager uses. My comments are about the design around it.
1. Remove CameraImageBase.default_data_type. On develop, nothing reads it anymore, and its own docstring calls it a "Legacy data-type alias". It is also what caused this bug: image_normals set default_data_type = "normals", which looked like enough, but the base ignores the attribute after #8127. Removing it from the base and the four subclasses leaves the __call__ signature as the only place a default is defined, so the next image term can't fall into the same trap.
2. This fix and the release fix take different approaches. #8373 (merged into release/3.0.0) falls back in the base with cfg.params.get("data_type", self.default_data_type) and does not add a data_type argument to image_normals. As a result, params={"data_type": "normals"} fails the manager's parameter check on 3.0 GA but works on develop. That split is probably fine for GA, but please state it in the PR description. If you apply (1), only apply it on develop, because the release fallback still reads the attribute.
Inline comments below cover the tests and the keyword-only argument.
| class TestModalityTerms: | ||
| """Per-modality reading, validation and normalization.""" | ||
|
|
||
| def test_normals_uses_default_data_type(self): |
There was a problem hiding this comment.
(AI-generated) This test only covers normals, but the bug is broader: any image term that doesn't declare data_type in its signature breaks. One test parametrized over all four terms would catch the next one, and it could replace both new tests:
@pytest.mark.parametrize(("term_cls", "data_type"), [
(image_rgb, "rgb"), (image_depth, "distance_to_image_plane"),
(image_normals, "normals"), (image_segmentation, "semantic_segmentation"),
])
def test_constructs_with_default_data_type(term_cls, data_type):
"""Each camera image term resolves its data type from its own signature when the cfg omits it."""
_, observe = _make_term(term_cls, _make_env({data_type: _random_rgb()}), normalize=False)
assert observe().shape[0] == NUM_ENVS| _, observe = _make_term(image_normals, _make_env({"normals": normals})) | ||
| torch.testing.assert_close(observe(), (normals + 1.0) * 0.5) | ||
|
|
||
| def test_normals_preserves_positional_normalize(self): |
There was a problem hiding this comment.
(AI-generated) Do we need this test? The manager always calls terms with keyword arguments, so this checks a calling convention rather than behavior a user would hit. Under the test-audit criteria I'd drop it, or fold it into the parametrized test above.
| channel_first: bool = False, | ||
| frame_stack: int = 1, | ||
| *, | ||
| data_type: str = "normals", |
There was a problem hiding this comment.
(AI-generated) Making this keyword-only is fine, because release/3.0.0 keeps the old positional order. But the other image terms take data_type as their second positional argument, so please add a sentence to the PR description explaining the difference.
Since isaac-sim#8127 the base class reads data_type from the term parameters, which the manager fills from each subclass's __call__ signature. The default_data_type attribute was no longer read and suggested a fallback that does not exist, which is how image_normals ended up without a data_type argument.
Replace the normals-only regression and the positional-call test with a sweep over CameraImageBase subclasses, so a new image term that omits data_type from __call__ fails here. Keep the normals normalization check, which has no other coverage.
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
Description
Adds the missing
data_type: str = "normals"parameter toimage_normalsand uses it to select the camera output. The new parameter is keyword-only so the existing positional order fornormalize,channel_first, andframe_stackremains compatible.Manager terms resolve callable defaults into
cfg.paramsbefore construction. Unlike the other per-modality image terms,image_normalsdid not declare its data type in the callable signature, so the strictCameraImageBaseconstructor raisedKeyError: 'data_type'. This change fixes the signature rather than adding a fallback to the base class, keeping default resolution consistent with #8127.Fixes NVBUG 6886551
No new dependencies.
Type of change
Release backport
developThe release-branch compatibility fix is already open as #8373, so an automated duplicate backport is not requested.
Screenshots
Not applicable; this fixes observation-term construction.
Validation
developbranch withKeyError: 'data_type'.KeyError: Falseand passes oncedata_typeis keyword-only.uv run isaaclab -f: passed, including the changelog gate againstupstream/develop.uv run --isolated --extra dev --extra ov -- make -C docs current-docs: passed without warnings.Checklist
pre-commitchecks withuv run isaaclab --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there