Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved per-primitive Warp test kernels and refactored raycast tests to use a single unified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| test.assertAlmostEqual(out_t.numpy()[0], expected, delta=delta) | ||
|
|
||
|
|
||
| def test_ray_intersect_mesh(test: TestRaycast, device: str): |
There was a problem hiding this comment.
🟡 After this refactor, test_ray_intersect_mesh launches kernel_test_geom with GeoType.MESH, which is the same dispatcher path already covered by test_mesh_ray_intersect_via_geom at lines 308-333. That leaves no direct coverage of the ray_intersect_mesh() entry point in this file, even though renderer code calls it directly at newton/_src/sensors/warp_raytrace/raytrace.py:439-446 and 680-687.
This distinction matters because ray_intersect_geom() hardcodes enable_backface_culling=False and _DEFAULT_MESH_MAX_T when it forwards to the helper (newton/_src/geometry/raycast.py:739-742), so a regression in the direct helper plumbing would slip past both tests here. The current test name and docstring also still read as if this test targets ray_intersect_mesh() itself.
Would you consider either restoring kernel_test_mesh so this test calls the helper directly, or renaming it to reflect that it is another ray_intersect_geom case and adding a separate direct ray_intersect_mesh() smoke test?
| test.assertAlmostEqual(out_t.numpy()[0], expected, delta=1e-5) | ||
|
|
||
|
|
||
| def test_geom_ray_intersect(test: TestRaycast, device: str): |
There was a problem hiding this comment.
⚪ After the refactor, every per-type test (test_ray_intersect_sphere, test_ray_intersect_box, and so on) also launches kernel_test_geom, so this function overlaps with them substantially. The sphere, box, capsule, cylinder, cone, and ellipsoid cases here use origin (-2, 0, 0) and direction (1, 0, 0), which match the hit case in each per-type test modulo delta. The only genuinely unique case is plane with origin (0, 0, 5) and direction (0, 0, -1) expecting 5.0, since test_ray_intersect_plane only uses 3-4-5 triples and never a unit-length axial ray.
Since this PR is a cleanup, consolidation would match that intent. A few options:
- remove
test_geom_ray_intersectentirely, - trim it to just the plane case,
- or fold the plane case into
test_ray_intersect_planeand delete this function.
If you keep it as-is, a short comment noting which coverage is unique would help future readers.
Description
Cleanup of raycast tests. Functionally similar but substantially more concise, and less kernels (avoiding excessive kernel compilation).
ray_intersect_geomis a dispatch function for the remaining ray functions and can be used across all tests.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Summary by CodeRabbit