Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d8de8fd
Fix gradient correctness defects in the differentiable rigid-body bac…
duburcqa Jul 21, 2026
7f289cf
Strengthen differentiable rigid-body gradient tests and align them wi…
duburcqa Jul 21, 2026
b3486d2
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
be4772f
Solve the constraint-solver adjoint system down to the round-off floor.
duburcqa Jul 22, 2026
ff014a4
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
7660ddf
Tighten the differentiability test tolerances to the measured cross-p…
duburcqa Jul 22, 2026
8a140aa
Align the differentiability tests and backward-pass comments with the…
duburcqa Jul 22, 2026
bc23657
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
bd98738
fixup! Solve the constraint-solver adjoint system down to the round-o…
duburcqa Jul 22, 2026
e89d0bf
fixup! Solve the constraint-solver adjoint system down to the round-o…
duburcqa Jul 22, 2026
ba8a21c
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
48e1afb
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
9666f31
Add the kernel fallback for grad buffers without zero-copy views and …
duburcqa Jul 22, 2026
6b9d8cd
fixup! Fix gradient correctness defects in the differentiable rigid-b…
duburcqa Jul 22, 2026
eaa815f
Branch the constraint-adjoint grad shuffles on the zero-copy support …
duburcqa Jul 22, 2026
f46d15c
Update the coding guidelines.
duburcqa Jul 22, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
- **Always pass `transpose=True`** to `qd_to_numpy` / `qd_to_torch` to move the batch dimension to front (`[B, n, dim]`), aligning with public getter conventions.
- **Hoist conversions before loops.** Call `qd_to_numpy` / `qd_to_torch` once, then index the local array.
- **`qd_to_numpy` with `copy=False` fails on CUDA.** GPU data always requires a copy to numpy.
- **Store `qd_to_torch(..., copy=False)` views in named locals**, one local per view; never chain the in-place op on the call expression. Drop `transpose=True` for element-wise in-place ops between two field views.
- **Metal stream sync is the caller's job**: after a batch of torch zero-copy writes, call `torch.mps.synchronize()` once before the next quadrants kernel reads the buffers. Helpers performing such writes (e.g. `qd_zero_grad`) never sync internally.
- **Branch zero-copy paths on `gs.use_zerocopy` alone; no try/except probing.**
- **Do not pass `copy=` at all** to `qd_to_torch` / `qd_to_numpy` when the value returned to the caller is fresh arithmetic (nothing internal aliases out) — let it zero-copy or copy only as needed. Reserve `copy=True` for when a raw field view would otherwise escape and could be mutated.
- **Store Python references at build time** (entity, link objects in dicts) for runtime lookup, rather than re-deriving from solver fields every step.
- **Never pass numpy scalar types** (e.g., `numpy.float64`) to kernel arguments. Always cast to native Python types (`float()`, `int()`). Numpy scalars break quadrants fastcache (weak reference).
Expand All @@ -58,6 +61,7 @@
- **Rigid kernel and func calls are positional for self-named arguments**, relying on the canonical parameter order below; anonymous constants (bare literals such as trailing static flags) are passed by keyword. A func call that passes a struct member alongside its parent struct stays keyword-only, because quadrants' positional func-argument expansion duplicates the member.
- **Canonical parameter order, for every kernel and func:** indices of any kind first (loop indices, index tensors like `envs_idx`, index-valued scalars like `joint_idx` or range starts/ends), then dynamic native Python scalars and fixed-size quadrants vectors (per-call values: positions, quaternions, penetrations), then the kernel/func-specific tensors, then all state structs, then all info structs, then the static configs, then the kernel/func-specific constants-in-practice (shape integers, eps, tolerances - info by nature even when not declared as such), and the kernel/func-specific static compilation flags (`qd.template()` booleans) at the very end - except `errno`, which systematically takes the last position. Within the state/info/config groups the component order is fixed - dyn, rigid, collider (mpr, gjk, support_field, sdf), constraint - with leaf structs at their aggregate's slot in its declared field order. Call sites pass arguments in signature order.
- **Rigid solver** is the reference implementation for kernel and code quality standards.
- **No dtype in quadrants fixed-size vector annotations**: `qd.types.vector(3)`, never `qd.types.vector(3, dtype=gs.qd_float)`.
- **Pure read-write data accessors on the hot path go through zero-copy views, keeping the kernel only as fallback.** A tiny kernel that merely shuffles or rewrites a field (payload remap, flag sweep) pays full kernel dispatch for microseconds of work: when `gs.use_zerocopy` holds, do it in place through a `qd_to_torch(..., copy=False)` / `qd_to_numpy(..., copy=False)` view, and fall back to the kernel otherwise. Reserve dedicated kernels for genuine computation.

## API Design
Expand Down Expand Up @@ -114,6 +118,7 @@
- **Step budget (hard).** Use the strict minimum steps needed, measured not guessed; if the checked quantity is FP-sensitive, pad by whichever is larger: +10% or round up to 50. Per-test horizon tiers: `<100` fine, `100-300` grey (needs justification), `>300` near-prohibited (only 4-5 such tests in the whole suite). Also bound total env-steps (`steps * n_envs`) - a 500-step x 16-env settle (8000) is unacceptable; delete it or fit the budget. CI cost multiplies every step across the matrix.
- **Validate the math, not scale.** One constraint proves the theory (works for 1 => works for 1k); no large/high-DOF scene is needed for correctness. A real multi-body scene earns its steps only when it validates something a single constraint cannot (end-to-end stability, e.g. the bowl tower).
- **Prefer FP-robust checks.** A single constraint-solve comparison (one step from a fixed state) is robust; a multi-step trajectory comparison across numerically-distinct codepaths (different solver arms/factor paths) is not - fp32 compounds and correct paths diverge. Compare a single solve, or an analytical physical property. Cross-engine (MuJoCo) consistency = the real-world check; analytical closed-form = the math check. Regression tests are reactive (add when a bug surfaces), not speculative.
- **Gradient-vs-FD tolerances are pinned to measured floors**: floor T = max|ana - fd| / (1 + |fd|) at the configured eps, worst across cpu and BOTH GPU archs; tolerance = 1.5x-5x the floor, values only {1, 2, 5}e-X; do-not-chase floors 1e-10 (fp64) / 5e-5 (fp32); eps per precision (large for fp32, small for fp64). A floor of exactly 0 = vacuous check - fix the loss, not the tolerance. An eps-independent residual is an analytical bug, never an FD artifact.
- **Tighten toward physically-exact conditions; slack hides bugs.** A padded safety factor or generous tolerance that makes a scenario pass trivially also masks real behavioral differences (across backends/arms/factor paths). Prefer the tightest thresholds the physics justifies. When tightening a test makes it start failing, investigate the root, don't back off. And verify a fix against the EXACT assertion path (full horizon, all phases), never a cheaper proxy - the part a proxy drops is where behavior diverges.

### Scene construction in tests and examples
Expand Down
9 changes: 7 additions & 2 deletions CODING_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ This document describes how we write, test, and review code in Genesis. It is ai
Two things to keep in mind throughout:

- **The rigid solver is the reference implementation.** When in doubt about naming, kernel structure, or code organization, open `genesis/engine/solvers/rigid/` and imitate what you find there. The historical rationale for the naming scheme is documented in [PR #1053](https://github.com/Genesis-Embodied-AI/Genesis/pull/1053).
- **Match the established sibling pattern; do not invent structure.** Before adding any helper, fixture, option, cast, comment, or file layout, look at how a sibling in the same directory already does it, and copy that. Genuine novelty is rarely the problem in reviews; deviation from an existing convention almost always is.
- **Match the established sibling pattern; do not invent structure.** Before adding any helper, fixture, option, cast, comment, or file layout, look at how a sibling in the same directory already does it, and copy that. Genuine novelty is rarely the problem in reviews; deviation from an existing convention almost always is. When a sibling contradicts an explicit rule in this document, the rule wins: legacy remnants are not a license.

## 1. Philosophy and priorities

Expand Down Expand Up @@ -136,6 +136,9 @@ Genesis strongly resists the accumulation of small wrappers. Helpers accrete, ge
- **Always pass `transpose=True`** so the batch dimension comes first (`[B, n, dim]`), matching the public getter convention.
- **Hoist conversions out of loops.** Convert once, then index the local array. If only a slice is needed, pass the slice as an argument to the conversion rather than slicing the full result afterward.
- Do not pass `copy=` at all when the returned value is fresh arithmetic; let the function zero-copy or copy as needed. Reserve `copy=True` for the case where a raw field view would otherwise escape and could be mutated later. Note that `copy=False` fails on CUDA: GPU data always requires a copy to reach numpy.
- **Store `qd_to_torch(..., copy=False)` views in named locals** before operating on them, one local per view; never chain the in-place op on the call expression. The named local documents what the view aliases and keeps each zero-copy acquisition visible on its own line. For element-wise in-place ops between two field views, drop `transpose=True`: both sides share the raw field layout, so transposing both is a no-op that only makes the views non-contiguous (batch-first is for values returned to consumers).
- **Metal stream sync is the caller's job.** torch (MPS) and quadrants share no compute stream on Metal: after a batch of torch zero-copy writes, call `torch.mps.synchronize()` once before the next quadrants kernel reads the buffers. Helpers performing such writes (e.g. `qd_zero_grad`) never sync internally, so a caller can batch many of them under a single flush.
- **Branch zero-copy paths on `gs.use_zerocopy` alone; no try/except probing.** Every platform gate (backend, torch version, device consistency) is resolved at `gs.init`, array_class buffers are standalone dense allocations with DLPack-supported dtypes, and allocations are capped at int32 element count at build time - so for these buffers the flag decides availability exactly, and exception-driven fallbacks are speculative defensive code.
- **Zero-copy aliasing hazard:** on the CPU backend, `qd_to_numpy` returns a live view into the field buffer. If you store the result across simulation steps, `.copy()` it first (or reduce it to Python scalars immediately) - otherwise a later "delta" between stored and current values is identically zero, because both names alias the same memory.
- Prefer torch-based zero-copy to feed state into other solvers; numpy zero-copy only exists on the CPU backend.
- Handle all environments at once. Calling getters/setters per environment index in a loop is inefficient; batch the call, falling back to a loop only where an accessor fundamentally requires it. Avoid branching code on batched-versus-unbatched; handle it at the argument level (`envs_idx=env_idx if batched else None`).
Expand All @@ -148,6 +151,7 @@ Genesis strongly resists the accumulation of small wrappers. Helpers accrete, ge
- **New kernels are free functions decorated with `@qd.kernel`** - no `@qd.data_oriented` classes. Use `V_ANNOTATION` from `genesis.utils.array_class` for type-polymorphic parameters. The FEM solver is the historical exception: it follows the old `@qd.data_oriented` method pattern, and kernels added there must stay consistent with it. Never put `@qd.data_oriented` on non-solver classes (materials, couplers, entities).
- **Call rigid kernels and funcs positionally for self-named arguments.** The canonical parameter order below makes those call sites unambiguous without keyword names. Anonymous constants (bare literals, typically the trailing static flags) are passed by keyword so their meaning shows at the call site, and every argument after the first keyword one is keyword too. Exception: numeric constants in module-local qd.func calls stay positional - forcing the keyword there cascades onto every following argument and wrecks the compactness of internal call chains for no readability gain. One exception: a func call that passes a struct member alongside its parent struct (e.g. `constraint_state.qacc` next to `constraint_state`) stays keyword-only, because quadrants' positional func-argument expansion duplicates the member in the flattened call. Kernel calls from Python are unaffected.
- **Parameters follow the canonical component order.** Indices of any kind come first - loop indices, index tensors (`envs_idx`, `dofs_idx`), index-valued scalars (`joint_idx`, geom ids, range starts/ends) - then the dynamic native Python scalars and fixed-size quadrants vectors (per-call values: positions, quaternions, penetrations, step sizes), then the tensors specific to the kernel/func (raw inputs/outputs, errno), then every state struct, then every info struct, then the static configs, then the kernel/func-specific constants-in-practice (shape integers, eps, tolerances - info by nature even when not declared as such), and finally the kernel/func-specific static compilation flags (`qd.template()` booleans such as `is_backward`). One exception: `errno` systematically takes the last position. The order keeps what matters most in front and sorts the rest by constant-ness (how likely each argument is to change between calls); since the constants-in-practice cluster at the tail, an anonymous constant can take its keyword without forcing keyword form onto the arguments that matter. Within the state/info/config groups the component order is fixed - dyn, rigid, collider (mpr, gjk, support_field, sdf), constraint - with leaf structs at their aggregate's slot in its declared field order, and adjoint-cache instances right after their primary. Within a group the guideline mandates nothing further (beyond the fixed component order of the struct groups): keep the existing relative order, and never reshuffle a signature more than the guideline requires. One predictable order makes every signature readable at a glance, keeps positional call sites unambiguous, and turns argument mismatches into immediate compile errors instead of silent misbindings.
- **No dtype in quadrants fixed-size vector annotations**: `qd.types.vector(3)`, never `qd.types.vector(3, dtype=gs.qd_float)`. Annotations are evaluated once at import and never re-evaluated after destroy / re-init, so a captured dtype goes stale when precision changes between inits.
- **The outermost `for` loop of a kernel is auto-parallelized.** Outer-scope locals mutated inside that loop are a single shared memory slot that all threads race on; the value after the loop is undefined and run-to-run unstable on GPU. The CPU backend often looks correct only because its default executor is single-threaded - do not let that fool you. To fix: serialize the loop (wrap the body in `for _ in range(1):`, or use `qd.loop_config(serialize=True)`), or use `qd.atomic_min`/`qd.atomic_max`/`qd.atomic_add` for pure reductions. When something "works on CPU but fails on Metal", check for this pattern before suspecting a compiler bug.
- **Loop-variable scoping:** a variable assigned in the outer scope of a kernel (for example a lane-strided `i_c_ = tid; while i_c_ < n: ...`) cannot be reused as a `for` loop variable in the same kernel; the compiler rejects it. The trailing-underscore convention for relative indices conveniently keeps these names distinct.
- **Do not pass the same buffer under two different argument names of a single kernel.** Intra-kernel argument aliasing miscompiles. Use dedicated buffers.
Expand Down Expand Up @@ -227,6 +231,7 @@ Genesis strongly resists the accumulation of small wrappers. Helpers accrete, ge
- If your change makes an existing test fail and you believe the test's expectation is what should change, raise it with the maintainers with your analysis; do not silently rewrite assertions.
- **Never remove or weaken an existing assertion or measurement to silence a failure**, including a CI-only failure. A threshold that only holds on one machine is a calibration problem to raise with data, not a license to delete the check.
- **Tighten toward physically-exact conditions; slack hides bugs.** A padded safety factor or a generous tolerance that makes a scenario pass trivially also masks real behavioral differences - between solver backends, arms, or factor paths. Prefer the tightest thresholds the physics justifies: an over-pushed or loosely-checked scenario passes on every backend while a genuine discrepancy sits underneath it. When tightening a test toward the exact expected behavior makes it start failing, that failure is the test working as intended - a newly-observable issue to investigate at its root, not a calibration to back off.
- **Gradient-vs-FD tolerances are pinned to measured floors.** Floor T = max|ana - fd| / (1 + |fd|) at the configured eps, worst across platforms - cpu and BOTH GPU archs, whose fp32 floors can differ by 4x. Tolerance = 1.5x-5x the floor, values only {1, 2, 5}e-X, with do-not-chase floors 1e-10 (fp64) / 5e-5 (fp32) and per-precision eps (fp32 needs a large step to clear its noise floor, fp64 a small one to bound truncation). A floor of exactly 0 means the check is vacuous (constant observable) - fix the loss, not the tolerance. An eps-independent residual is an analytical bug (missing adjoint term or wrong backward primal), never an FD artifact.
- **Verify a fix against the exact assertion path, not a cheaper proxy.** A reduced reproduction - fewer steps, only the first phase, a simpler scene - can pass while the full test still fails, because the part it drops is exactly where the behavior diverges (a longer settle, a second load phase). Confirm on the real test at the real horizon before declaring a fix validated.

### 12.6 Step budget and minimality
Expand All @@ -243,6 +248,6 @@ Genesis strongly resists the accumulation of small wrappers. Helpers accrete, ge
- **Commit messages are a single title line**: a plain sentence ending with a period, without the PR bracket tag. No body, no implementation details, no co-author trailers.
- **No AI attribution anywhere.** No "Generated with ..." footers, no AI co-author trailers, in commits, PR bodies, issues, or comments.
- **PR titles carry exactly one bracket tag from a closed set and end with a period.** The tags are `[BUG FIX]`, `[FEATURE]`, `[MISC]` (everything else, including performance and refactor work: `[MISC] Speed up ...`), `[CHANGING]` (a default-behavior change), and `[BREAKING]` (an API break). Inventing a tag (`[PERF]`) is a review-blocking error.
- **PR titles state the benefit for end users, not the implementation.** PR titles are used to generate the changelog automatically, so each title must be clear and enjoyable to read for end users and let them understand what improvement to expect: `[MISC] Speed up forward kinematics on GPU.`, not `[MISC] Remove unnecessary atomic add from root COM accumulation.`. The mechanism belongs in the PR description.
- **PR titles state the benefit for end users, not the implementation.** PR titles are used to generate the changelog automatically, so each title must be clear and enjoyable to read for end users and let them understand what improvement to expect: `[MISC] Speed up forward kinematics on GPU.`, not `[MISC] Remove unnecessary atomic add from root COM accumulation.`. The mechanism belongs in the PR description, and so does every testing / validation detail (tolerances, platforms, suites) - never the title.
- **PR descriptions are terse and go straight to the point.** State the change and the one fact that justifies it; cut every sentence a reviewer can infer. Long descriptions do not get read. Follow the PR template (`.github/pull_request_template.md`), keeping each section tight and removing sections that are empty.
- **Search for existing related issues and track them in the PR description**, even when you are not the author of those issues. Link each one in the `Related Issue` section with the accurate keyword: `Resolves` only when the PR fully closes it, otherwise `Addresses` with a word on which part is covered. This keeps the issue tracker in sync with the work actually landing.
Loading
Loading