[API] Return ProxyArray from FrameView.get_scales#5712
Conversation
BaseFrameView.get_scales returned a raw wp.array while get_world_poses and get_local_poses returned ProxyArray. The mixed contract surprised consumers reading scales alongside poses; align the three getters on ProxyArray. Update the USD, Fabric, Newton, and OvPhysX backends to wrap their returns. The Fabric FabricFrameView -> UsdFrameView sync path now extracts .warp before feeding the array to set_scales. Extend the FrameView contract test to assert get_scales returns ProxyArray, parallel to the existing pose assertions.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Automated Code Review
PR: [API] Return ProxyArray from FrameView.get_scales
Commit: b128323
✅ Summary
This PR aligns get_scales() with the existing pose getters by returning ProxyArray instead of raw wp.array. The change improves API consistency across all four backends (USD, Fabric/PhysX, Newton, OvPhysX).
🏗️ Architecture Assessment
Strengths:
- Consistent return type across all
BaseFrameViewimplementations - Follows the established pattern from
get_world_poses()andget_local_poses() - Clean delegation in
OvPhysxFrameView.get_scales()— the method just returns the underlyingUsdFrameViewresult, which now correctly propagates theProxyArray
Implementation Notes:
UsdFrameView: Wraps the constructedwp.arrayinProxyArrayat the end ✓FabricFrameView: Wraps kernel output appropriately ✓NewtonSiteFrameView: Wraps gathered output ✓OvPhysxFrameView: Signature updated, delegates correctly ✓
🔍 Code Quality
| Aspect | Status | Notes |
|---|---|---|
| Type annotations | ✅ | Return types updated to ProxyArray |
| Docstrings | ✅ | Updated to reference ProxyArray with proper Sphinx cross-refs |
| Class-level docs | ✅ | "Pose getters" → "All getters" language updated across backends |
| Internal call sites | ✅ | FabricFrameView._sync_fabric_from_usd_once extracts .warp correctly |
🧪 Test Coverage
Good:
test_return_types_are_torcharrayextended to coverget_scales()andget_scales(indices)- Test comment updated to reflect "pose and scale getters"
Suggestions:
- Consider adding a round-trip test verifying
set_scales(view.get_scales().warp)works correctly - The test name
test_return_types_are_torcharrayis now slightly misleading since it testsProxyArray— consider renaming totest_return_types_are_proxyarrayin a follow-up
📋 Changelog Fragments
All four packages have proper changelog entries:
isaaclab/changelog.d/jichuanh-scales-proxyarray.rst✓isaaclab_newton/changelog.d/jichuanh-scales-proxyarray.rst✓isaaclab_ovphysx/changelog.d/jichuanh-scales-proxyarray.rst✓isaaclab_physx/changelog.d/jichuanh-scales-proxyarray.rst✓
Breaking change is clearly documented with migration guidance.
⚠️ Breaking Change Impact
The PR correctly identifies this as a breaking change. The impact is:
- Code calling
.numpy()directly on the return value will break → use.warp.numpy()or.torch.numpy() - Code using
wp.to_torch(result)is unaffected (can use.torchaccessor instead) - Code passing directly to Warp kernels or
set_scales()needs.warpextraction
The grep audit finding no external callers in source/ and scripts/ is reassuring.
🚦 CI Status
- Pre-commit: ✅ Passing
- Changelog check: ✅ Passing
- Build wheel: ✅ Passing
- Docs build: ⏳ Pending
- Contract tests: ⏳ Pending (awaiting CI infrastructure)
📝 Minor Suggestions (Non-blocking)
-
FabricFrameView.get_scales (line 281): The method has type hints in the signature but the
scalesandindicesparameters inset_scales(line 274) don't — consider adding them for consistency in a follow-up. -
Test naming:
test_return_types_are_torcharray→test_return_types_are_proxyarraywould be more accurate.
✅ Verdict
Looks good to merge once CI completes. The implementation is clean, consistent with the existing codebase patterns, well-documented, and properly tested. The breaking change is appropriately flagged with clear migration guidance.
Greptile SummaryThis PR aligns
Confidence Score: 4/5The change is a clean, consistent wrapping of raw The wrapping is mechanical and symmetrical across backends. The only finding is that the updated class-level docstrings now say "All getters return ProxyArray" while The class-level docstrings in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["get_scales(indices)"] --> B{Backend?}
B -- "USD" --> C["UsdFrameView\n build wp.array from USD xformOp:scale\n wrap in ProxyArray"]
B -- "Fabric/PhysX" --> D{use_fabric?}
D -- "No" --> C
D -- "Yes" --> E["FabricFrameView\n launch Warp kernel → scales_wp\n wrap in ProxyArray"]
B -- "Newton" --> F["NewtonSiteFrameView\n launch Warp kernel → out\n wrap in ProxyArray"]
B -- "OvPhysX" --> G["OvPhysxFrameView\n delegate to UsdFrameView"]
G --> C
C --> H["ProxyArray\n .warp → wp.array\n .torch → torch.Tensor"]
E --> H
F --> H
style H fill:#d4edda,stroke:#28a745
Reviews (1): Last reviewed commit: "Return ProxyArray from FrameView.get_sca..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| implementation at runtime based on the active physics backend. | ||
|
|
||
| All pose getters return :class:`~isaaclab.utils.warp.ProxyArray`. Setters accept ``wp.array``. | ||
| All getters return :class:`~isaaclab.utils.warp.ProxyArray`. Setters accept ``wp.array``. |
There was a problem hiding this comment.
The class docstring was changed from "All pose getters" to "All getters", but get_visibility (across all backends) still returns torch.Tensor, not ProxyArray. Any reader relying on this class-level contract to infer the return type of get_visibility will be misled. The same inaccuracy propagates to the matching docstring updates in UsdFrameView, NewtonSiteFrameView, and OvPhysxFrameView.
…oxyarray # Conflicts: # source/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py
There was a problem hiding this comment.
🤖 Automated Code Review (Updated)
PR: [API] Return ProxyArray from FrameView.get_scales
Commit: e29cc29
✅ Summary
Clean, well-scoped API consistency PR. Aligns get_scales() with the existing pose getters by returning ProxyArray instead of raw wp.array across all four backends. The merge conflict in newton_site_frame_view.py was resolved correctly.
🏗️ Architecture & Correctness
All backends implement the contract correctly:
UsdFrameView: Wraps constructedwp.arrayinProxyArray✓FabricFrameView(Fabric path): Wraps kernel output inProxyArray✓FabricFrameView(USD fallback): Delegates toUsdFrameView.get_scales()which already returnsProxyArray— no double-wrapping ✓NewtonSiteFrameView: Wraps gathered output inProxyArray✓OvPhysxFrameView: Delegates toUsdFrameView— propagation correct ✓
Internal call site fix:
FabricFrameView._sync_fabric_from_usd_once correctly extracts .warp before passing to set_scales(), matching the existing pattern for pose data two lines above. ✓
⚠️ Findings
1. Inconsistent class docstring in FabricFrameView (Minor)
base_frame_view.py, usd_frame_view.py, and ovphysx_frame_view.py all updated their class-level docstrings from "Pose getters" to "Getters" (or "All getters"). However, fabric_frame_view.py still reads:
Pose getters return :class:`~isaaclab.utils.warp.ProxyArray`. Setters accept ``wp.array``.
This should be updated to match the other backends for consistency.
2. Test function name is now misleading (Nitpick)
test_return_types_are_torcharray now asserts ProxyArray for poses and scales. The name suggests it tests for TorchArray. Consider renaming to test_return_types_are_proxyarray in a follow-up for clarity.
🧪 Test Coverage
Good:
- Contract test extended with
get_scales()andget_scales(indices)assertions - Parametrized across
cpuandcuda:0devices
Suggestion (non-blocking):
- A round-trip test (
set_scales(view.get_scales().warp, ...)followed by verify) would exercise the exact path fixed in_sync_fabric_from_usd_onceand guard against future regressions.
📋 Changelog & Breaking Change
All four packages have correct changelog fragments with clear migration guidance. The breaking-change classification is appropriate.
🚦 CI Status
- Pre-commit: ✅ Passing
- Changelog check: ✅ Passing
- Build wheel: ✅ Passing
- Remaining checks: ⏳ Pending
✅ Verdict
Looks good to merge once CI completes. Only one minor inconsistency (Finding #1 — the FabricFrameView class docstring) is worth fixing before merge. The rest are non-blocking suggestions for follow-up.
| self._fabric_usd_sync_done = True | ||
|
|
||
| def get_scales(self, indices=None): | ||
| def get_scales(self, indices: wp.array | None = None) -> ProxyArray: |
There was a problem hiding this comment.
Nit: The class-level docstring (line 22 of this file) still says Pose getters return :class:~isaaclab.utils.warp.ProxyArray`` — should be updated to Getters return for consistency with `BaseFrameView`, `UsdFrameView`, and `OvPhysxFrameView` which were all updated in this PR.
3 participants
Summary
BaseFrameView.get_scaleswith the pose getters: returnsProxyArrayinstead of rawwp.arrayacross all four backends (USD, Fabric/PhysX, Newton, OvPhysX).get_world_poses/get_local_poses.1. Motivation
BaseFrameView.get_world_posesandget_local_posesalready returnProxyArray(giving callers.warpand.torchaccessors), butget_scalesstill returned a rawwp.array. The mixed contract surprised callers reading scales alongside poses; this PR aligns the three getters on a single return type.2. Changes
2.1 Contract
BaseFrameView.get_scalesreturn annotation changed fromwp.arraytoProxyArray; docstring updated. Class-level "getters return ProxyArray" doc broadened from "pose getters".2.2 Backends
UsdFrameView.get_scaleswraps the constructedwp.arrayinProxyArray.FabricFrameView.get_scaleswraps the kernel output inProxyArrayand types the signature explicitly.NewtonSiteFrameView.get_scaleswraps the gathered output inProxyArray.OvPhysxFrameView.get_scales(delegating toUsdFrameView) updates the signature and docstring.2.3 Internal call sites
FabricFrameView._sync_fabric_from_usd_oncereadsself._usd_view.get_scales().warpbefore feeding the array intoset_scales— matches the existing.warpextraction pattern used for poses two lines above.2.4 Tests
frame_view_contract_utils.test_return_types_are_torcharrayextended to assertget_scales()andget_scales(indices)returnProxyArray— parallel to the existing pose-getter assertions.3. Backward compatibility
Marked Breaking in the changelog fragments. Wider grep across
source/andscripts/found no external callers ofget_scales— only the internalFabricFrameView._sync_fabric_from_usd_oncepath (fixed in this PR). Downstream code that reads.numpy()directly on the previous return will need to extract.warpfirst; code that already uses the deprecation bridge orwp.to_torch(...)is unaffected.4. Test plan
./isaaclab.sh -f(pre-commit) — clean.