Fix selection slice shapes#792
Conversation
📝 WalkthroughWalkthroughUpdates selection utility to handle integer indices directly, adjust empty-slice construction without pointer offset, preserve trailing dimensions for empty slices, and avoid copies. Test updated to use the anymal base path. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Sel as selection._get_attribute_array
participant WP as wp.array
Caller->>Sel: _get_attribute_array(attrib, _slice)
alt _slice is int
Sel-->>Caller: return attrib[:, _slice]
else _slice is slice
alt empty slice (start==stop)
Sel->>WP: construct array(shape=(attrib.shape[0], 0, *attrib.shape[2:]), ptr=attrib.ptr, strides=attrib.strides, device, pinned, copy=False)
WP-->>Sel: zero-width view
Sel-->>Caller: return view
else non-empty slice
Sel-->>Caller: process as existing slice path
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)newton/tests/test_anymal_reset.py (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/_src/utils/selection.py (2)
548-559: Type hints drift: acceptSlice | int | Nonein public helpers.Both helpers now legitimately pass
intthrough to_get_attribute_array. Update the annotations to match and reduce static‑analysis noise.- def _get_attribute_values(self, name: str, source: Model | State | Control, _slice: slice | None = None): + def _get_attribute_values(self, name: str, source: Model | State | Control, _slice: Slice | int | None = None): - def _set_attribute_values( - self, name: str, target: Model | State | Control, values, mask=None, _slice: slice | None = None - ): + def _set_attribute_values( + self, name: str, target: Model | State | Control, values, mask=None, _slice: Slice | int | None = None + ):
168-171: CI failure: make the “no matches” error actionable (and double‑check test patterns).GPU tests failed with
KeyError: 'No matching articulations'. Improve the message to include the pattern to speed up triage, and verify the test uses a valid pattern on CI fixtures.- raise KeyError("No matching articulations") + raise KeyError(f"No matching articulations for pattern '{pattern}'")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/utils/selection.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
newton/_src/utils/selection.py
521-521: Avoid specifying long messages outside the exception class
(TRY003)
526-526: Shebang should contain python, pytest, or uv run
(EXE003)
526-526: Shebang is present but file is not executable
(EXE001)
526-526: Shebang should be at the beginning of the file
(EXE005)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/_src/utils/selection.py
[error] 169-169: KeyError: 'No matching articulations' raised during ArticulationView initialization.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (2)
newton/_src/utils/selection.py (2)
519-521: Int indexing path: shape collapse is intentional—please confirm call‑site expectations and add tests.Returning
attrib[:, _slice]forintcollapses the frequency axis. This looks correct for fixed/floating‑base roots, but please:
- Verify negative indices behave as expected across CPU/GPU.
- Add shape assertions/tests (e.g., transform/spatial_vector/float dtypes).
526-536: Empty‑slice construction LGTM; fix linter false positive and confirm stride safety.
- Good: preserving trailing dims and using
copy=Falsewith baseptravoids copies and keeps shapes consistent.- Lint:
#!is parsed as a shebang mid‑file. Replace with plain#to silence EXE00x.Apply this diff:
- #! workaround for empty slice until this is fixed: https://github.com/NVIDIA/warp/issues/958 + # workaround for empty slice until this is fixed: https://github.com/NVIDIA/warp/issues/958Also, please run a quick GPU/CPU test to ensure a zero-length, strided view is accepted by Warp kernels for float/transform/spatial_vector.
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Bug Fixes
Performance
Tests