Simplify private FeatherPGS API to matrix-free-only path#2432
Draft
dylanturpin wants to merge 7 commits intonewton-physics:mainfrom
Draft
Simplify private FeatherPGS API to matrix-free-only path#2432dylanturpin wants to merge 7 commits intonewton-physics:mainfrom
dylanturpin wants to merge 7 commits intonewton-physics:mainfrom
Conversation
Remove the private solver mode and per-stage kernel selection knobs\nfrom SolverFeatherPGS and run the live step path as matrix-free\nonly. This keeps the private API aligned with the current winner\npath instead of preserving branch-local ablation surface area.\n\nAdd focused unit coverage for the stripped constructor signature and\na minimal smoke step, and keep a human-review PR description draft in\n.agent/review using the published nightly gh-pages artifacts.
Drop the unused dense-only and standalone matrix-free helper paths\nfrom the private FeatherPGS line so the branch reflects the\nshipped winner path only.\n\nAlso update the living ExecPlan and PR draft polish to match this\nMilestone 2 pass and keep the review artifacts accurate.
Rebase the private FeatherPGS branch onto upstream/main and remove the\nremaining dense-mode presentation from the solver surface. Rename the\nlast dense_* constructor and helper names to mode-neutral matrix-free\narticulated-row terms, refresh the focused signature coverage, and\nupdate the ExecPlan and PR draft so their validation evidence matches\nthe rebased branch state.
3 tasks
Contributor
📝 WalkthroughWalkthroughThis PR introduces matrix-free-only variants of the FeatherPGS solver, removing kernel selection and dense contact handling parameters. A new solver module is added with public exports, velocity conversion logic in Featherstone kernels is refactored to support origin-referenced propagation, and tests validate the matrix-free behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Caller
participant FK as eval_single_articulation_fk_with_velocity_conversion
participant Prop as Velocity Propagation
participant Out as State Output
Test->>FK: Call with joint_qd (COM convention for FREE/DISTANCE)
FK->>FK: Step 1: Adjust joint linear_vel<br/>linear_vel_adj = linear_vel - (ω × r_com)
FK->>Prop: Propagate with adjusted<br/>velocity (origin-referenced)
Prop->>Prop: Compose v_wc = v_wpj + [linear_vel_adj, ω]
Prop->>Out: Return propagated<br/>origin-referenced twist v_wc
Out->>Out: Step 2: Convert back to COM<br/>v_com = v_wc_origin + (ω × r_com)
Out->>Test: Write body_qd as [v_com, ω]<br/>for FREE/DISTANCE joints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/solvers/feather_pgs/__init__.py`:
- Line 1: The SPDX header at the top of the new feather_pgs module uses the
wrong creation year (shows 2025); update the SPDX-FileCopyrightText line so the
single-year value is 2026 (do not create a range), i.e., edit the SPDX header
token to reflect the file's actual creation year.
In `@newton/_src/solvers/featherstone/kernels.py`:
- Around line 1861-1901: The FK recursion is mixing origin-referenced and
COM-referenced twists by writing COM-converted velocities into body_qd for
JointType.FREE/DISTANCE during recursion; instead keep body_qd origin-referenced
throughout eval_single_articulation_fk_with_velocity_conversion so parent reads
(v_wpj reconstruction from body_qd[parent]) are consistent. Concretely: in the
block that currently computes v_origin/omega/r_com/v_com and sets
body_qd[child], always write the origin-referenced spatial twist v_wc to body_qd
(remove the in-recursion COM conversion for FREE/DISTANCE); perform the
FREE/DISTANCE child COM conversion only later in the separate post-pass /
writeback kernel. Ensure references: body_qd, v_wpj, v_wc, body_com, X_wc, and
JointType.FREE / JointType.DISTANCE are the targets of this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 95961d2d-1fe7-4e06-a020-15f6a2848924
📒 Files selected for processing (6)
.agent/execplans/fpgs-private-api-matrix-free.mdnewton/_src/solvers/feather_pgs/__init__.pynewton/_src/solvers/feather_pgs/kernels.pynewton/_src/solvers/feather_pgs/solver_feather_pgs.pynewton/_src/solvers/featherstone/kernels.pynewton/tests/test_feather_pgs.py
| @@ -0,0 +1,20 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers | |||
Contributor
There was a problem hiding this comment.
Fix the SPDX creation year for this new file.
This file is introduced in this PR, so Line 1 should use 2026, not 2025; otherwise the header records the wrong creation year.
As per coding guidelines: "In SPDX copyright lines, use the year the file was first created. Do not create date ranges or update the year when modifying a file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/solvers/feather_pgs/__init__.py` at line 1, The SPDX header at
the top of the new feather_pgs module uses the wrong creation year (shows 2025);
update the SPDX-FileCopyrightText line so the single-year value is 2026 (do not
create a range), i.e., edit the SPDX header token to reflect the file's actual
creation year.
Comment on lines
+1861
to
+1901
| v_wpj = wp.spatial_vector() | ||
| if parent >= 0: | ||
| X_wp = body_q[parent] | ||
| X_wpj = X_wp * X_wpj | ||
| r_p = wp.transform_get_translation(X_wpj) - wp.transform_point(X_wp, body_com[parent]) | ||
|
|
||
| v_wp = body_qd[parent] | ||
| w_p = wp.spatial_bottom(v_wp) | ||
| v_p = wp.spatial_top(v_wp) + wp.cross(w_p, r_p) | ||
| v_wpj = wp.spatial_vector(v_p, w_p) | ||
|
|
||
| # transform from world to joint anchor frame at child body | ||
| X_wcj = X_wpj * X_j | ||
| # transform from world to child body frame | ||
| X_wc = X_wcj * wp.transform_inverse(X_cj) | ||
|
|
||
| x_child_origin = wp.transform_get_translation(X_wc) | ||
| v_parent_origin = wp.vec3() | ||
| w_parent = wp.vec3() | ||
| if parent >= 0: | ||
| v_wp = body_qd[parent] | ||
| w_parent = wp.spatial_bottom(v_wp) | ||
| v_parent_origin = com_twist_to_point_velocity(v_wp, X_wp, body_com[parent], x_child_origin) | ||
| linear_joint_world = wp.transform_vector(X_wpj, wp.spatial_top(v_j)) | ||
| angular_joint_world = wp.transform_vector(X_wpj, wp.spatial_bottom(v_j)) | ||
| linear_vel = wp.transform_vector(X_wpj, wp.spatial_top(v_j)) | ||
| angular_vel = wp.transform_vector(X_wpj, wp.spatial_bottom(v_j)) | ||
|
|
||
| if type == JointType.FREE or type == JointType.DISTANCE: | ||
| v_j_world = transform_twist(X_wpj, v_j) | ||
| linear_joint_origin = velocity_at_point(v_j_world, x_child_origin) | ||
| angular_joint_world = wp.spatial_bottom(v_j_world) | ||
| else: | ||
| child_origin_offset_world = x_child_origin - wp.transform_get_translation(X_wcj) | ||
| linear_joint_origin = linear_joint_world + wp.cross(angular_joint_world, child_origin_offset_world) | ||
| # Public FREE/DISTANCE joint_qd stores the linear term at the child COM. | ||
| # Convert back to the child-body origin convention before composing the | ||
| # world-space twist, then convert back to COM on body_qd writeback below. | ||
| r_com = wp.quat_rotate(wp.transform_get_rotation(X_wc), body_com[child]) | ||
| linear_vel = linear_vel - wp.cross(angular_vel, r_com) | ||
|
|
||
| v_wc_origin = wp.spatial_vector(v_parent_origin + linear_joint_origin, w_parent + angular_joint_world) | ||
| v_wc = v_wpj + wp.spatial_vector(linear_vel, angular_vel) | ||
|
|
||
| body_q[child] = X_wc | ||
| body_qd[child] = origin_twist_to_com_twist(v_wc_origin, X_wc, body_com[child]) | ||
|
|
||
| # Velocity conversion for FREE and DISTANCE joints: | ||
| # v_wc is a spatial twist at the origin, but body_qd should store COM velocity | ||
| # Transform: v_com = v_origin + ω x r_com | ||
| if type == JointType.FREE or type == JointType.DISTANCE: | ||
| v_origin = wp.spatial_top(v_wc) | ||
| omega = wp.spatial_bottom(v_wc) | ||
| r_com = wp.quat_rotate(wp.transform_get_rotation(X_wc), body_com[child]) | ||
| v_com = v_origin + wp.cross(omega, r_com) | ||
| body_qd[child] = wp.spatial_vector(v_com, omega) | ||
| else: | ||
| body_qd[child] = v_wc |
Contributor
There was a problem hiding this comment.
Keep FK recursion in one twist representation.
Lines 1865-1870 now reconstruct v_wpj from body_qd[parent] after Lines 1894-1899 have already converted FREE/DISTANCE children to COM velocity, while non-FREE parents still stay origin-referenced. That makes the parent readback representation-dependent, and descendants of articulated links with non-zero body_com will get the wrong propagated twist. Keep an internal origin-referenced body_qd through the whole recursion and only convert FREE/DISTANCE bodies on a final post-pass / writeback stage.
Based on learnings: "during eval_single_articulation_fk_with_velocity_conversion, body_qd[parent] holds an origin-referenced spatial twist ... throughout the FK recursion. The FREE/DISTANCE COM conversion ... is applied only in the separate post-pass kernel."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/solvers/featherstone/kernels.py` around lines 1861 - 1901, The FK
recursion is mixing origin-referenced and COM-referenced twists by writing
COM-converted velocities into body_qd for JointType.FREE/DISTANCE during
recursion; instead keep body_qd origin-referenced throughout
eval_single_articulation_fk_with_velocity_conversion so parent reads (v_wpj
reconstruction from body_qd[parent]) are consistent. Concretely: in the block
that currently computes v_origin/omega/r_com/v_com and sets body_qd[child],
always write the origin-referenced spatial twist v_wc to body_qd (remove the
in-recursion COM conversion for FREE/DISTANCE); perform the FREE/DISTANCE child
COM conversion only later in the separate post-pass / writeback kernel. Ensure
references: body_qd, v_wpj, v_wc, body_com, X_wc, and JointType.FREE /
JointType.DISTANCE are the targets of this change.
❌ 22 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR narrows the private FeatherPGS API line to the current matrix-free path instead of keeping the older dense and split ablation surface alive.
It removes the private solver constructor knobs for
pgs_modeand per-stage kernel selection, renames the remaining articulated-row tuning surface fromdense_*to mode-neutralcontact_complianceandmax_constraints, and runs the private FeatherPGS step path as matrix-free only.The rationale matches the published nightly ablations from
2026-04-01T20-49-30Zongh-pages: the matrix-free path materially outperforms split on the tabletop ablation, and the tiledhinv_jt/ tiled PGS / parallel-stream path already represents the current winner direction for the private line.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
New feature / API change
Summary by CodeRabbit
New Features
Refactor
Tests