[#165] Deterministic math: M2.0 Scalar Foundation #167
Conversation
Implement quarter-wave LUT + linear interpolation trig backend (no platform libm). Add golden-vector trig tests; keep error-budget audit test ignored. Update deterministic math docs + decision log.
Cargo updated the lockfile during local checks; commit to keep the branch clean.
Adds a dev-only binary for regenerating the quarter-wave sin table used by deterministic trig.
Route Mat4/Quat trig through the deterministic LUT backend and add a CI guard that forbids raw trig calls in warp-core math modules.
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds deterministic math to warp-core: LUT-backed F32 trig and LUT generator, an optional Q32.32 fixed-point scalar lane (DFix64), canonicalized motion payload v2 (Q32.32) with v0 compatibility and upgrade-on-write, CI expansions (macOS/MUSL/feature lanes), a guard script forbidding raw trig, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Exec as Motion Executor
participant Atom as AtomPayload
participant Decode as Decoder\n(v0/v2)
participant Scalar as Scalar Backend\n(F32Scalar / DFix64)
participant Encode as Encoder\n(v2 canonical)
Exec->>Atom: read payload bytes
Atom->>Decode: hand bytes
Decode->>Decode: detect length → v0 or v2
Decode->>Scalar: produce raw pos_raw, vel_raw (i64 or f32)
Scalar->>Scalar: scalar_from_raw → scalar arrays
Scalar->>Scalar: compute new_pos = pos + vel
Scalar->>Scalar: canonicalize NaN→0, flush subnormals, saturate ∞
Scalar->>Encode: scalar_to_raw → new raw arrays
Encode->>Encode: encode as v2 (Q32.32) and set v2 type_id
Encode->>Exec: return updated AtomPayload
sequenceDiagram
autonumber
participant Caller as sin_cos_f32()
participant Range as RangeReduce\n(quadrant)
participant LUT as trig_lut\n(SIN_QTR_LUT_BITS)
participant Interp as sin_qtr_interp
participant Quad as QuadrantReconstruct
participant Canon as CanonicalizeZero
participant Out as (sin, cos)
Caller->>Range: abs(angle).rem_euclid(TAU)
Range->>Range: quadrant, qtr_angle
Range->>LUT: index = floor(qtr_frac * SEGMENTS)
LUT->>Interp: read adjacent samples
Interp->>Quad: interpolated quarter-wave value
Quad->>Canon: apply symmetry & sign
Canon->>Out: coerce -0.0→+0.0, return (sin, cos)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/echo-session-ws-gateway/src/main.rs (1)
845-920: No tests for the new metrics instrumentation.The existing tests cover frame validation thoroughly, but the substantial new metrics system (
GatewayMetrics,alloc_conn,remove_conn,observe_message,snapshot,hex32) has zero test coverage. Given that metrics correctness affects operational visibility:
hex32— trivial to unit testalloc_conn/remove_conn— verify conn_id allocation, active count, warp subscription cleanupobserve_message— verify counter increments, warp/conn cross-referencingsnapshot— verify serialization matches expected structureAt minimum, add unit tests for the pure functions. Integration tests for the metrics endpoint can come later.
crates/warp-core/src/math/mat4.rs (3)
68-75: Signed-zero canonicalization lacks explanatory comment.Lines 69-71 canonicalize
-0.0to+0.0for determinism (since(-0.0 == 0.0)is true in IEEE 754, but the bit patterns differ). While this is correct, the intent is non-obvious to future maintainers who may see this as redundant code.Suggested comment to clarify intent
pub fn rotation_x(angle: f32) -> Self { let (s_raw, c_raw) = trig::sin_cos_f32(angle); + // Canonicalize -0.0 to +0.0 for deterministic bit-level equality. let s = if s_raw == 0.0 { 0.0 } else { s_raw }; let c = if c_raw == 0.0 { 0.0 } else { c_raw }; let ns = if s == 0.0 { 0.0 } else { -s };
83-90: Identical canonicalization pattern inrotation_y.The signed-zero canonicalization at lines 84-86 mirrors
rotation_x. Consider extracting this pattern into a helper function if it appears in multiple locations (already seen in 3 rotation methods + quat.rs).Potential helper extraction
Add to
crates/warp-core/src/math/trig.rs:/// Canonicalizes -0.0 to +0.0 for deterministic bit-level equality. #[inline] pub(crate) fn canonicalize_zero(value: f32) -> f32 { if value == 0.0 { 0.0 } else { value } }Then use:
pub fn rotation_y(angle: f32) -> Self { let (s_raw, c_raw) = trig::sin_cos_f32(angle); - let s = if s_raw == 0.0 { 0.0 } else { s_raw }; - let c = if c_raw == 0.0 { 0.0 } else { c_raw }; + let s = trig::canonicalize_zero(s_raw); + let c = trig::canonicalize_zero(c_raw); let ns = if s == 0.0 { 0.0 } else { -s };
98-105: Third instance of canonicalization pattern inrotation_z.Lines 99-101 repeat the same signed-zero canonicalization. This reinforces the recommendation to extract a helper (see
rotation_ycomment).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/ci.ymlcrates/echo-session-ws-gateway/Cargo.tomlcrates/echo-session-ws-gateway/README.mdcrates/echo-session-ws-gateway/assets/dashboard.htmlcrates/echo-session-ws-gateway/src/main.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/bin/gen_sin_qtr_lut.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/math/fixed_q32_32.rscrates/warp-core/src/math/mat4.rscrates/warp-core/src/math/mod.rscrates/warp-core/src/math/quat.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/math/trig.rscrates/warp-core/src/math/trig_lut.rscrates/warp-core/src/payload.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/tests/dfix64_tests.rscrates/warp-core/tests/engine_motion_negative_tests.rscrates/warp-core/tests/math_rotation_tests.rscrates/warp-core/tests/math_scalar_tests.rscrates/warp-core/tests/proptest_seed_pinning.rsdocs/SPEC_DETERMINISTIC_MATH.mddocs/decision-log.mddocs/execution-plan.mddocs/notes/f32scalar-deterministic-trig-implementation-guide.mdscripts/check_no_raw_trig.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/tests/math_rotation_tests.rscrates/warp-core/tests/dfix64_tests.rscrates/warp-core/src/math/trig.rscrates/warp-core/tests/engine_motion_negative_tests.rscrates/warp-core/src/payload.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/src/bin/gen_sin_qtr_lut.rscrates/echo-session-ws-gateway/src/main.rscrates/warp-core/src/math/fixed_q32_32.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/math/trig_lut.rscrates/warp-core/src/math/quat.rscrates/warp-core/tests/math_scalar_tests.rscrates/warp-core/src/math/mod.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/math/mat4.rscrates/warp-core/tests/proptest_seed_pinning.rs
🧠 Learnings (3)
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Run `cargo clippy --all-targets -- -D missing_docs` before every PR; CI will expect a zero-warning, fully documented surface.
Applied to files:
scripts/check_no_raw_trig.sh.github/workflows/ci.yml
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
crates/warp-core/tests/dfix64_tests.rscrates/warp-core/src/math/scalar.rs
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
🧬 Code graph analysis (6)
crates/warp-core/tests/dfix64_tests.rs (1)
crates/warp-core/src/math/scalar.rs (7)
raw(292-294)from_f32(90-90)from_f32(208-210)from_f32(407-409)to_f32(96-96)to_f32(212-214)to_f32(411-413)
crates/warp-core/tests/engine_motion_negative_tests.rs (2)
crates/warp-core/src/payload.rs (2)
decode_motion_atom_payload(161-169)motion_payload_type_id(31-33)crates/warp-core/src/attachment.rs (3)
new(144-146)new(257-262)new(305-307)
crates/warp-core/tests/deterministic_sin_cos_tests.rs (1)
crates/warp-core/src/math/scalar.rs (1)
new(141-155)
crates/echo-session-ws-gateway/src/main.rs (1)
crates/echo-session-proto/src/wire.rs (1)
decode_message(263-293)
crates/warp-core/src/math/quat.rs (1)
crates/warp-core/src/math/trig.rs (1)
sin_cos_f32(24-53)
crates/warp-core/src/math/mat4.rs (1)
crates/warp-core/src/math/trig.rs (1)
sin_cos_f32(24-53)
🪛 LanguageTool
docs/notes/f32scalar-deterministic-trig-implementation-guide.md
[grammar] ~95-~95: Ensure spelling is correct
Context: ...eterministic table index + fraction. 3. Lookup adjacent samples and interpolate. 4. Ap...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~300-~300: ‘by accident’ might be wordy. Consider a shorter alternative.
Context: ...latform libm in generation or runtime by accident. - Accidentally introducing -0.0 at q...
(EN_WORDINESS_PREMIUM_BY_ACCIDENT)
🪛 markdownlint-cli2 (0.18.1)
docs/SPEC_DETERMINISTIC_MATH.md
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (macOS)
- GitHub Check: Cargo Audit (stable)
- GitHub Check: Cargo Audit (stable)
🔇 Additional comments (75)
crates/echo-session-ws-gateway/Cargo.toml (3)
20-20: Dependency verified.The path
../echo-session-protoexists and declaresversion = "0.1.0", matching the specification. No action required.
20-22: Scope misalignment remains: Gateway infrastructure bundled with deterministic math PR.Dependencies ARE actively used—
echo-session-protois imported and pattern-matched in the gateway's message handling (lines 26, 227-233), andserdedecorates five structs with Serialize derives (lines 30, 86, 116, 122, 136, 145).futures-utilprovides streaming utilities (line 29). This is not frivolous bloat.However, the core concern stands: Why does a PR titled "Deterministic math: M2.0 Scalar Foundation" include WebSocket gateway infrastructure changes? The PR objectives mention LUT-backed trig, Q32.32 fixed-point, and motion payload v2—zero mention of gateway work. The AI summary references "metrics instrumentation," which also doesn't appear in stated objectives.
Two scenarios:
- Gateway work IS part of M2.0 Scalar → PR description is incomplete. Update it to justify bundling infra changes with math foundation work.
- Gateway work is orthogonal → This belongs in a separate PR. Don't conflate concerns.
The code is sound; the project structure is the problem. Fix the story, not the code.
22-22: No issue. Loose versioning inCargo.tomlwith a committedCargo.lockis standard Rust practice and guarantees deterministic builds.The version spec
"1.0"inCargo.tomlis correct here. TheCargo.lockfile pinning serde to1.0.228is what ensures reproducible builds—not theCargo.tomlversion spec. This pattern is consistent across all 9 workspace crates and is the idiomatic approach. Pinning exact versions inCargo.tomlwould be an anti-pattern that breaks workspace consistency.Likely an incorrect or invalid review comment.
crates/echo-session-ws-gateway/assets/dashboard.html (4)
293-323: D3 data join is correctly implemented.The enter/update/exit pattern with a proper key function (
d.warp_id) ensures DOM reconciliation is efficient and correct. Null-coalescing fallbacks for optional fields prevent rendering crashes.
325-394: Full chart redraw is acceptable at 1 Hz / 90 points.Tearing down and rebuilding the entire SVG every tick is not ideal for high-frequency updates, but at 1 Hz with 90 data points, the overhead is negligible. If you later increase the poll rate, consider incremental path updates instead.
437-447: Dynamic D3 loading with error feedback is well-implemented.Loading D3 from the gateway's
/vendor/path enables offline operation. Theonerrorhandler surfaces load failures visibly rather than failing silently.
9-22: CSS color palette passes all WCAG AA contrast requirements.All tested color pairs exceed 4.5:1 minimum for normal text:
--texton--bgachieves 16.27:1,--mutedon--panelachieves 8.10:1, and--mutedon--panel-2achieves 8.26:1. The dark theme is accessibility-compliant with no contrast issues.crates/echo-session-ws-gateway/src/main.rs (7)
1-4: License header and module documentation are present and correct.SPDX identifier and copyright notice follow the required format per coding guidelines.
54-84: Metrics data structures are well-designed for the tracking requirements.
ConnMetricsandWarpMetricsuseHashSetfor O(1) membership operations. The fields capture the right observability surface without over-collecting.
198-242: Message observation logic correctly updates both per-connection and per-warp metrics.The match arms exhaustively handle all
SessionMessagevariants and properly maintain bidirectional subscription tracking between connections and warps.
305-313: Manual hex encoding is correct and dependency-free.The LUT approach is cache-friendly and produces consistent lowercase output. No need to pull in a crate for 8 lines.
459-463:Access-Control-Allow-Origin: *is intentionally permissive.The comment explains this enables consumption from local dev servers. For a diagnostic dashboard on an internal gateway, this is acceptable. If the gateway ever exposes sensitive operational data, revisit this.
564-584: Per-frame lock acquisition is correct — decode outside lock is good.Decoding happens before acquiring the lock, so CPU-bound CBOR parsing doesn't extend the critical section. The frame is counted regardless of decode success, which is the right accounting.
756-759: Connection cleanup correctly placed after task teardown.
remove_connis called after all spawned tasks are aborted/joined, ensuring the connection is fully dead before removing it from metrics. This prevents stale warp subscription references.docs/execution-plan.md (1)
38-56: LGTM - Clear documentation of completed milestones.The "Today's Intent" entries provide excellent evidence trails with specific test commands and commit references. The format makes it easy for future work to validate that the claimed work is actually complete and passing.
Minor note: You're documenting "COMPLETED" work in the PR that implements it - consider whether "IN PROGRESS → COMPLETED after merge" would be more accurate for the PR state, but this is a nitpick on a living document.
docs/SPEC_DETERMINISTIC_MATH.md (1)
56-83: LGTM - Excellent CI parity documentation.The new "Local Validation" section provides clear, actionable commands that developers can run locally to match CI behavior. The three-lane structure (default float, det_fixed, MUSL) is well-organized and the explanations are concise.
This is exactly what a deterministic-math spec needs: boring, reproducible validation commands.
crates/warp-core/src/math/fixed_q32_32.rs (3)
19-41: LGTM - Correct ties-to-even rounding implementation.The ties-to-even logic is textbook correct:
- Line 32-34: Clear majority cases
- Line 36-37: Properly rounds ties toward the even quotient
This ensures deterministic rounding behavior matching IEEE 754 default.
119-158: LGTM - Correct Q32.32 to f32 conversion with proper edge cases.The numerical logic is solid:
- Line 128: Correctly canonicalizes -0.0 → +0.0
- Lines 140-146: Proper significand construction with ties-to-even rounding
- Lines 149-152: Handles the rounding overflow case (1.111... → 10.0)
- Lines 154-157: Correct IEEE 754 bit pattern assembly
Same minor nitpick about excessive
try_fromusage as flagged in thefrom_f32review, but the core conversion is correct.
16-17: Feature gate forONE_RAWis correct; no inconsistency exists.
ONE_RAWis used exclusively within theDFix64impl block (scalar.rs:274), which is itself feature-gated behind#[cfg(feature = "det_fixed")]. The conversion functionsfrom_f32andto_f32do not depend onONE_RAWand operate as standalone utilities. The feature gate is properly scoped to its single consumer.docs/decision-log.md (1)
9-12: LGTM - Well-documented decisions with clear rationale.The four new decision entries provide excellent context for the M2.0 deterministic math work:
- Motion payload v2 determinism rationale is clear
- DFix64 feature-gating strategy is well-justified
- Guardrails enforcement makes sense
- Trig stability approach is documented
Each entry includes concrete consequences and explains the trade-offs made. This is exactly what a decision log should contain.
crates/warp-core/src/bin/gen_sin_qtr_lut.rs (1)
18-18: The original review comment incorrectly flags a mismatch that does not exist.gen_sin_qtr_lut.rsline 18 (SEGMENTS = 1024) andtrig_lut.rsline 12 (SIN_QTR_SEGMENTS = 1024) are perfectly aligned. The generator correctly outputs code using the symbolSIN_QTR_SEGMENTS + 1, and the constant values match. Additionally, the file satisfies all coding guidelines: SPDX header and copyright are present, and the module-level rustdoc documents the utility's purpose.Likely an incorrect or invalid review comment.
crates/warp-core/tests/math_rotation_tests.rs (1)
9-11: Tolerance adjustment is justified for LUT-backed trig.The relaxation from
1e-7to2e-7accounts for linear interpolation error in the deterministic quarter-wave LUT. The comment clearly explains the rationale. For unit-scale rotations at cardinal angles (π/2), this tolerance remains tight and appropriate.crates/warp-core/src/math/mod.rs (2)
11-12: Internal module visibility is correct.The
pub(crate)visibility forfixed_q32_32appropriately restricts usage to withinwarp-core, preventing external dependence on internal fixed-point conversion details.
17-18: Private trig modules prevent external coupling.The
trigandtrig_lutmodules are correctly kept private (nopub), ensuring external crates cannot bypass the deterministic backend by calling internal trig primitives directly. This enforces the architectural boundary described in the PR objectives.crates/warp-core/src/math/mat4.rs (1)
4-4: Import is clean and minimal.The
use crate::math::trig;import is appropriately scoped and brings in only the required deterministic trig backend.crates/warp-core/src/math/quat.rs (1)
4-4: Trig import added correctly.The import of
trigalongside existing math types is clean and follows the module structure.crates/warp-core/tests/math_scalar_tests.rs (1)
26-27: Deterministic bit patterns are correctly verified at π boundary.The LUT-backed implementation correctly produces the exact IEEE 754 bit patterns expected by the assertions:
sin(π) → 0x00000000(+0.0f32): Quadrant 2 range-reduction yieldsa = 0.0, samplingSIN_QTR_LUT[0]directly.cos(π) → 0xbf800000(-1.0f32): Interpolation withπ/2triggers the boundary guard at line 65 (if t >= 1024.0), returning exactly1.0, which quadrant symmetry negates to-1.0.The implementation includes explicit safeguarding (line 65–68) that prevents any out-of-bounds LUT access at the
π/2boundary. The test file complies with all coding guidelines: SPDX header, copyright attribution, and appropriate test-scoped#![allow(missing_docs)]suppression.scripts/check_no_raw_trig.sh (4)
1-14: LGTM: Script foundation is solid.The shebang, SPDX headers, strict error handling (
set -euo pipefail), repository root detection, and ripgrep availability check are all properly implemented.
16-23: Well-documented policy scope.The inline comments clearly articulate the rationale for the guardrail and explicitly enumerate the sanctioned exceptions. This makes the enforcement boundary transparent.
25-31: Search pattern is appropriately conservative.The regex
\.(sin|cos|sin_cos)\(will match method-style invocations. While it may flag occurrences in comments or string literals (false positives), this conservative approach is acceptable for a CI guardrail—it's safer to be overly cautious and manually review exceptions than to miss violations.
33-39: Error handling and reporting are correct.The script properly reports violations to stderr with context (the matching lines), exits with code 1 to fail CI, and provides a clear success message when no violations are found.
crates/warp-core/tests/dfix64_tests.rs (9)
1-5: Headers and feature gating are correct.SPDX license headers are present as required. The
#![allow(missing_docs)]is appropriate for a test module, and the feature gate ensures these tests only compile whendet_fixedis enabled.
7-9: LGTM: Imports are minimal and appropriate.
10-14: Constant validation is correct for Q32.32 format.Verifies that
ZEROencodes as0x0andONEencodes as0x0000_0001_0000_0000, which is correct for a 32.32 fixed-point representation.
16-26: Conversion test cases are mathematically correct.All expected Q32.32 encodings are accurate:
- Signed zero canonicalization to positive zero
- Integer and fractional values map correctly to their fixed-point representations
28-35: Round-trip test is correct with canonicalization semantics.The test verifies that basic values round-trip through
from_f32→to_f32. Note that-0.0is canonicalized to+0.0duringfrom_f32(as verified in line 19), and IEEE 754 equality treats them as equal, so the assertion passes correctly.
37-41: Infinity saturation behavior is well-defined.Mapping
±∞toi64::MAX/i64::MINis a sensible saturation strategy for fixed-point conversions, preventing undefined behavior or panics.
43-46: NaN canonicalization is deterministic.Mapping
NaNto zero is consistent with the deterministic scalar canonicalization policy and prevents non-deterministic NaN propagation.
48-56: Arithmetic validation is appropriately scoped.The test name
basic_arithmetic_is_reasonablecorrectly signals that these are sanity checks rather than exact fixed-point arithmetic proofs. Comparing the f32-converted results to direct f32 arithmetic is a pragmatic validation strategy.
58-63: Exact trig validation at zero is crucial.This test verifies bit-exact results for the critical boundary case
sin_cos(0.0), ensuring the deterministic trig backend produces the mathematically exact fixed-point representations (0for sine,1 << 32for cosine).crates/warp-core/src/math/trig.rs (5)
1-18: Excellent module documentation and headers.The SPDX headers are present as required, and the module-level rustdoc clearly articulates the determinism goals, implementation strategy, and reasoning for avoiding platform transcendentals. This sets clear expectations for maintainers.
20-28: Function documentation and non-finite guard are well-implemented.The rustdoc explicitly documents edge-case behavior, and the early-exit guard for non-finite inputs prevents downstream NaN/infinity propagation through the LUT logic.
29-53: Range reduction and quadrant symmetry are mathematically rigorous.The implementation correctly:
- Uses
rem_euclidfor canonical range reduction to[0, TAU)- Avoids floating-point division hazards by using direct comparisons for quadrant determination
- Applies the identity
cos(a) = sin(π/2 - a)to compute cosine from the sine LUT- Reconstructs full-wave results via textbook quadrant sign/swap symmetries
All quadrant mappings verified against trigonometric identities. This is exemplary work.
55-62: Defensive bounds check is prudent.The
sin_qtr_interpfunction includes a guard against out-of-range inputs, even though upstream logic should guaranteeangle_qtr ∈ [0, π/2]. This defensive posture protects against future refactoring hazards and makes the function's contract explicit.
63-77: Linear interpolation implementation is correct with appropriate safeguards.The normalization, endpoint handling, and interpolation logic are all sound:
- Index computation and fractional extraction are standard
- Explicit endpoint check for
sin(π/2) = 1.0prevents off-by-one errors- Clippy allows for casts are justified by the bounds guarantees
- Assumes the LUT provides
N+1samples to supporti0 + 1lookups without overflow (verified in caller logic)docs/notes/f32scalar-deterministic-trig-implementation-guide.md (4)
26-86: Goals, non-goals, and determinism contract are clearly articulated.The documented API contract for input canonicalization, output requirements, and non-finite handling aligns exactly with the implementation in
crates/warp-core/src/math/trig.rs. This clarity will prevent future implementation drift.
88-268: Implementation guide is comprehensive and accurate.The step-by-step plan correctly describes the LUT-backed deterministic trig architecture, and each step aligns with the actual implementation in
trig.rsandtrig_lut.rs. The guidance on range reduction, interpolation, symmetries, and canonicalization is technically sound and matches the reviewed code.
270-310: LUT generation guidance and completion criteria are pragmatic.The recommendation to commit the LUT as data (Strategy A) correctly prioritizes build reproducibility and audit-ability over build-time generation. The pitfalls checklist covers common determinism hazards (boundary cases, platform libm leakage, -0.0/subnormal handling), and the completion criteria are concrete and testable.
1-2: > Likely an incorrect or invalid review comment.crates/warp-core/tests/proptest_seed_pinning.rs (1)
71-86: LGTM — Canonicalization logic is rigorous.The double encode/decode round-trip to derive
expected_posandexpected_velcorrectly validates that the motion rule operates on quantized values and produces bit-identical results. Usingto_bits()for comparison is the right call for determinism verification.crates/warp-core/tests/deterministic_sin_cos_tests.rs (2)
25-45: LGTM — ULP distance calculation is correct.The ordered-float mapping correctly handles the sign-magnitude quirks of IEEE 754, ensuring
abs_diffproduces the true ULP distance. The NaN sentinel ofu32::MAXis appropriate.
102-120: LGTM — Canonical invariant sweep is thorough.The test correctly validates that all outputs satisfy the determinism contract (no
-0.0, no subnormals, canonical NaNs only) across a dense angle grid. The finite-input → finite-output check on lines 113-116 is essential.crates/warp-core/src/math/trig_lut.rs (1)
22-152: LUT endpoints and length verified — LGTM.
- First entry
0x00000000=0.0✓- Last entry
0x3f800000=1.0✓- Array length: 1025 entries (indexed 0..=1024) ✓
The quarter-wave symmetry approach is sound. Storing raw bits avoids any float-literal parsing ambiguity across toolchains.
crates/warp-core/tests/engine_motion_negative_tests.rs (3)
25-31: LGTM — v0 encoder correctly avoids canonicalization.The helper faithfully produces raw v0 payloads without canonicalization, allowing the upgrade path to be tested. This is the right approach.
102-120: LGTM — NaN canonicalization test is correct.The test correctly validates that NaN inputs are mapped to zero before the motion rule executes, resulting in
pos = [0, 0, 3]andvel = [0, 0, 2].
122-138: LGTM — Infinity saturation test validates extrema correctly.The saturated Q32.32 extrema (
i64::MAX / 2^32 ≈ 2^31) are correctly represented. The test confirms that overflow arithmetic saturates rather than wraps..github/workflows/ci.yml (3)
100-128: LGTM — macOS test coverage is welcome.The new
test-macosandtest-macos-det-fixedjobs provide cross-platform coverage. Usingmacos-latestis reasonable for catching platform-specific issues, though be aware it may drift over time.
130-162: LGTM — Comprehensivedet_fixedcoverage across libc variants.Testing
det_fixedon glibc, musl, and macOS ensures the deterministic math implementation doesn't rely on platform-specific behavior. This is exactly what a determinism-focused PR needs.
243-258:scripts/check_no_raw_trig.shexists, is executable, and properly implemented.The script is committed to the repository with execute permissions, so the CI job's error handling is defensive cover for future mistakes—not a current blocker. The script correctly enforces the deterministic math guard policy: forbidding raw trig calls (
.sin(),.cos(),.sin_cos()) outside of the sanctionedscalar.rs,trig.rs, andtrig_lut.rsfiles withincrates/warp-core/src/math. No action required.crates/warp-core/src/payload.rs (3)
96-108: LGTM — v2 decoder is correct.The 8-byte chunk iteration and Q32.32 → f32 conversion are implemented correctly. The defensive
try_into().ok()?is harmless given the prior length check.
217-226: LGTM — Round-trip tests validate exact value preservation.The test correctly verifies that values like
1.0,2.0,0.5survive the Q32.32 encoding round-trip exactly. Usingassert_eq!on[f32; 3]is safe here because these are exactly representable in Q32.32.
50-57: All claims verified; code meets guidelines.The function correctly delegates non-finite handling through
fixed_q32_32::from_f32, which is properly documented (lines 53–58 infixed_q32_32.rs) and implements exactly the specified semantics: NaN → 0, ±∞ → saturated extrema (i64::MAX/MIN). The rustdoc onencode_motion_payload(lines 40–48) explicitly documents these invariants for callers.Both
payload.rsandfixed_q32_32.rscarry correct SPDX headers and all public APIs are fully documented. Q32.32 → LE bytes serialization is straightforward and correct.crates/warp-core/src/math/scalar.rs (5)
193-206: LGTM — F32Scalar trig correctly delegates to deterministic backend.The
sin,cos, andsin_cosimplementations properly delegate totrig::sin_cos_f32and re-canonicalize the results throughSelf::new(). This ensures outputs satisfy the invariants (no-0.0, no subnormals, canonical NaN).
326-346: LGTM — Q32.32 multiplication with ties-to-even is correctly implemented.The rounding logic on lines 333-336 correctly implements round-to-nearest with ties-to-even:
r > half→ round up unconditionally.r == half && (q & 1) == 1→ round up to make even (ties-to-even).The use of
unsigned_abs()and subsequent sign restoration handles negative products correctly.
392-405: Acknowledged precision tradeoff in DFix64 trig.The
sin/cosimplementations route through f32, which loses precision for DFix64 values outside f32's representable range. For typical angle inputs (small multiples of 2π), this is acceptable. A future optimization could implement native fixed-point trig, but the current approach satisfies the determinism contract.
318-324: LGTM — Negation correctly saturates fori64::MIN.The special case on line 319-320 handles the two's complement asymmetry correctly:
-i64::MINwould overflow, so it saturates toi64::MAX. This maintains the "all operations are total" contract.Also applies to: 452-459
252-269: LGTM — DFix64 documentation is thorough.The struct-level docs clearly explain the Q32.32 encoding, the determinism contract, and the semantic invariants. Public accessors are documented. This meets the "rustdoc on all public APIs" guideline.
crates/warp-core/src/demo/motion.rs (8)
1-2: LGTM: License headers are pristine.Perfect compliance with repository standards.
10-13: Import additions noted for dual-format payload support.The addition of
decode_motion_atom_payload_q32_32andencode_motion_payload_q32_32alongside the preserveddecode_motion_atom_payloadsignals intent to handle both legacy v0 and canonical v2 formats. Verify that the usage throughout the file maintains this dual-format contract consistently.
86-110: Scalar conversion and arithmetic logic is clean.The component-wise position update (
pos[i] = pos[i] + vel[i]) is correctly implemented using the abstracted scalar backend. Conversions viascalar_from_rawandscalar_to_raware symmetric and appropriate for bothdet_fixedand non-det_fixedconfigurations.Assuming the critical decode issue at line 82 is resolved, this logic is sound.
112-114: LGTM: Canonical v2 upgrade-on-write is correctly implemented.The pattern of always writing canonical v2 (Q32.32) with the correct
type_idensures gradual migration from legacy v0 to v2. This is the right approach for deterministic evolution.
117-122: Matcher correctly handles dual-format, but executor does not.The matcher uses
decode_motion_atom_payload, which handles both v0 and v2 formats. This is correct behavior—rules should match all valid payload encodings. However, the executor (line 82) does not honor this contract, creating the critical mismatch flagged above.
127-151: LGTM: Rule constructor and documentation are accurate.The updated documentation correctly describes deterministic scalar semantics and payload matching. The rule construction is standard and correct.
19-38: No actionable issues found—documentation claim about v0 legacy support is accurate.The
decode_motion_atom_payload_q32_32()function internally handles both v0 and v2 payload formats via type_id discrimination. When matching v0 payloads, it callsdecode_motion_payload_q32_32_v0()(which decodes f32 bytes and deterministically quantizes to Q32.32); when matching v2, it callsdecode_motion_payload_q32_32_v2(). The documentation correctly reflects this polymorphic behavior—no contract violation exists.
220-254: Test coverage for v0→v2 upgrade already exists; this specific unit test's scope is narrower than implied.The test at lines 220-254 creates only v2 payloads via
encode_motion_atom_payload, so it does not exercise v0 legacy decoding. However, the exact scenario you recommended—creating a v0 payload and verifying it decodes, executes, and upgrades to v2—is already thoroughly validated incrates/warp-core/tests/engine_motion_negative_tests.rsundermotion_v0_payload_is_accepted_and_upgraded_to_v2(), which:
- Constructs a raw v0 payload (24 bytes, 6×f32 LE) with type ID
payload/motion/v0- Runs the motion executor through the full engine
- Asserts the result upgrades to v2 format
- Validates position math (pos [1.0, 2.0, 3.0] + vel [0.5, -1.0, 0.25] → [1.5, 1.0, 3.25])
The unit test at lines 220-254 has a narrower scope (v2 encode/decode round-trip), which is appropriate for its file. The legacy v0 pathway is not missing—it is covered by integration-level tests. No action required.
|
Addressed CodeRabbit actionables for PR #167. Scope cleanup:
Deterministic math fixes:
Follow-ups:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (2)
crates/warp-core/Cargo.toml (1)
37-41: Clarify the stability status ofdet_fixedbeyond "test-only."The feature is labeled
(experimental, test-only), but the PR objectives, decision log, and CI configuration demonstrate thatdet_fixedis actively exercised across multiple platforms (glibc, musl, macOS) and used in the motion demo. This suggests it's a functional, continuously-tested backend rather than a throwaway test fixture.If
det_fixedis genuinely not production-ready, document why (e.g., "incomplete API coverage," "unoptimized performance," or "pending validation"). If it's production-ready but feature-gated for flexibility, drop the "test-only" qualifier and replace it with something like(feature-gated deterministic fixed-point backend)or(opt-in Q32.32 backend).The current phrasing creates ambiguity about whether downstream consumers should rely on this feature.
🔎 Suggested clarification
If the backend is functional but opt-in:
# Scalar backend lanes (CI/test orchestration). # - `det_float`: default float32-backed lane using `F32Scalar`. -# - `det_fixed`: fixed-point Q32.32 lane using `DFix64` (experimental, test-only). +# - `det_fixed`: fixed-point Q32.32 lane using `DFix64` (opt-in deterministic backend). det_float = [] det_fixed = []Or if there are known limitations:
-# - `det_fixed`: fixed-point Q32.32 lane using `DFix64` (experimental, test-only). +# - `det_fixed`: fixed-point Q32.32 lane using `DFix64` (experimental; limited transcendental coverage).docs/SPEC_DETERMINISTIC_MATH.md (1)
41-47: Markdown list indentation still flagged by linter.Static analysis (MD005/MD007) still complains about inconsistent indentation. The past review suggested 2-space nesting, which was applied, but the linter now expects 0-space indent for nested items under
*parents. Either:
- Suppress the lint for this file (pragmatic), or
- Restructure to use consistent
*markers without extra spacing.Given this is documentation and the structure is readable, this is low priority. Consider adding a
.markdownlint.jsonoverride for this file if the noise persists.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
crates/warp-core/Cargo.tomlcrates/warp-core/src/bin/gen_sin_qtr_lut.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/math/fixed_q32_32.rscrates/warp-core/src/math/mat4.rscrates/warp-core/src/math/quat.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/math/trig.rscrates/warp-core/src/math/trig_lut.rscrates/warp-core/src/payload.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/tests/engine_motion_negative_tests.rsdocs/SPEC_DETERMINISTIC_MATH.mddocs/decision-log.mddocs/execution-plan.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/tests/engine_motion_negative_tests.rscrates/warp-core/src/math/trig_lut.rscrates/warp-core/src/math/mat4.rscrates/warp-core/src/math/fixed_q32_32.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/math/trig.rscrates/warp-core/src/math/quat.rscrates/warp-core/src/payload.rscrates/warp-core/src/bin/gen_sin_qtr_lut.rs
🧠 Learnings (3)
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
crates/warp-core/src/math/trig_lut.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/demo/motion.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/payload.rs
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Run `cargo clippy --all-targets -- -D missing_docs` before every PR; CI will expect a zero-warning, fully documented surface.
Applied to files:
crates/warp-core/src/demo/motion.rs
🧬 Code graph analysis (8)
crates/warp-core/tests/deterministic_sin_cos_tests.rs (1)
crates/warp-core/src/math/scalar.rs (1)
new(141-155)
crates/warp-core/tests/engine_motion_negative_tests.rs (2)
crates/warp-core/src/payload.rs (2)
decode_motion_atom_payload(164-172)motion_payload_type_id(31-33)crates/warp-core/src/demo/motion.rs (1)
encode_motion_payload_v0_bytes(266-272)
crates/warp-core/src/math/mat4.rs (1)
crates/warp-core/src/math/trig.rs (2)
sin_cos_f32(34-78)canonicalize_zero(22-28)
crates/warp-core/src/demo/motion.rs (2)
crates/warp-core/src/payload.rs (6)
decode_motion_atom_payload(164-172)decode_motion_atom_payload_q32_32(180-188)encode_motion_payload_q32_32(64-70)motion_payload_type_id(31-33)encode_motion_payload(50-57)motion_payload_type_id_v0(22-24)crates/warp-core/src/math/fixed_q32_32.rs (2)
from_f32(83-141)to_f32(146-189)
crates/warp-core/src/math/scalar.rs (2)
crates/warp-core/src/math/trig.rs (1)
sin_cos_f32(34-78)crates/warp-core/src/math/fixed_q32_32.rs (3)
saturate_i128_to_i64(67-75)from_f32(83-141)to_f32(146-189)
crates/warp-core/src/math/trig.rs (1)
crates/warp-core/src/math/trig_lut.rs (1)
sin_qtr_sample(158-161)
crates/warp-core/src/math/quat.rs (1)
crates/warp-core/src/math/trig.rs (2)
sin_cos_f32(34-78)canonicalize_zero(22-28)
crates/warp-core/src/payload.rs (3)
crates/warp-core/src/attachment.rs (3)
new(144-146)new(257-262)new(305-307)crates/warp-core/src/ident.rs (1)
make_type_id(53-58)crates/warp-core/src/math/fixed_q32_32.rs (2)
from_f32(83-141)to_f32(146-189)
🪛 markdownlint-cli2 (0.18.1)
docs/SPEC_DETERMINISTIC_MATH.md
42-42: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (31)
docs/execution-plan.md (1)
38-66: Comprehensive documentation of the deterministic math milestone.The execution plan entries clearly document the scope, goals, and evidence for each completed work item. The format is consistent with the rest of the file, and the content aligns with the PR objectives and decision log. The evidence references (commit SHAs, test results, clippy outcomes) provide traceability.
Well done. No concerns here.
docs/decision-log.md (1)
9-13: Well-documented architectural decisions for deterministic math.The decision log entries provide clear rationale for the motion payload v2 upgrade, det_fixed backend, guardrails against raw trig, and LUT-backed F32Scalar trig. The consequence descriptions are specific and actionable.
This is exactly what decision logs should be. No issues.
crates/warp-core/src/demo/motion.rs (4)
40-76: Scalar backend abstraction is clean and well-documented.The conditional compilation provides a clear separation between the two backend paths:
det_fixedenabled: Direct Q32.32 ↔ DFix64 with no conversion lossdet_fixeddisabled: Q32.32 ↔ f32 ↔ F32Scalar with deterministic quantizationThe doc comments explain the conversion semantics clearly. The
pub(super)visibility is appropriate for internal wiring.Previous review concern about missing documentation has been fully addressed.
88-93: Legacy v0 support is correctly preserved.Previous review flagged this as CRITICAL, claiming that
decode_motion_atom_payload_q32_32only handled v2 payloads and would break v0 support. This concern is resolved.The implementation in
payload.rsshows thatdecode_motion_atom_payload_q32_32explicitly handles both:
motion_payload_type_id()→ v2 path (direct Q32.32 decode)motion_payload_type_id_v0()→ v0 path (f32 decode + quantization to Q32.32)The comment at lines 88-90 accurately describes this dual-format handling. The test at lines 274-308 validates the v0→v2 upgrade behavior. Legacy payloads are decoded, executed, and upgraded to v2 on write as documented.
Critical issue resolved. No further action required.
95-123: Position update and payload upgrade logic is correct.The scalar conversions follow the expected path:
- Decode raw Q32.32 → MotionScalar (backend-specific conversion)
- Component-wise position update:
pos[i] = pos[i] + vel[i]- Convert back to raw Q32.32
- Upgrade payload to v2 type_id and encode
The executor always writes v2 payloads regardless of input format, which converges the store toward the canonical encoding. This is the documented behavior per the decision log.
274-308: Test validates v0→v2 upgrade contract.The test constructs a legacy v0 payload (24-byte f32 encoding with
motion_payload_type_id_v0()), runs the executor, and asserts:
- Output type_id is v2 (
motion_payload_type_id())- Position/velocity values are correct (bit-exact comparison)
This proves that the executor accepts v0 inputs and upgrades them to v2, confirming the documented compatibility guarantee.
crates/warp-core/src/bin/gen_sin_qtr_lut.rs (1)
34-49: LGTM — output formatting is sound.Trailing commas are idiomatic Rust. The hex output with comments documenting the generator command is good practice for reproducibility.
crates/warp-core/tests/engine_motion_negative_tests.rs (1)
88-104: Test logic is correct.The upgrade path is properly validated: v0 → v2 type_id change, position updated by velocity, velocity preserved. Assertions are precise.
docs/SPEC_DETERMINISTIC_MATH.md (1)
56-83: Excellent addition of local validation instructions.The CI parity section with explicit
cargo testandcargo clippycommands fordet_float,det_fixed, and MUSL lanes is valuable for contributors. This reduces "works on my machine" drift.crates/warp-core/src/math/trig_lut.rs (2)
154-161: Doc comment added per past feedback — LGTM.The doc comment now specifies valid index range (
0..=SIN_QTR_SEGMENTS). Thedebug_assertprovides development-time safety; in release, an out-of-bounds index will trigger a standard array panic with a bounds error, which is acceptable for apub(crate)function.
22-152: LUT data spot-check passes.Verified:
SIN_QTR_LUT_BITS[0] = 0x00000000→0.0,SIN_QTR_LUT_BITS[1024] = 0x3f800000→1.0, andSIN_QTR_LUT_BITS[512] = 0x3f3504f3→≈0.7071068(sin(π/4)). Endpoints and midpoint are correct.crates/warp-core/tests/deterministic_sin_cos_tests.rs (3)
128-143: ±π/8 symmetry issue resolved.Past review flagged a 1-ULP asymmetry (
ef15vsef16). Now bothsin(π/8)andsin(-π/8)correctly use0x3ec3_ef15/0xbec3_ef15respectively — exact bit-for-bit negation as required for odd-function determinism. Well done.
25-45:ulp_diffimplementation is correct.The "ordered float" mapping correctly handles the discontinuity at zero. NaN handling returns
u32::MAX, which is appropriate for error budget calculations.
164-167: TODO tracking addressed per past feedback.
// TODO(#115)and updated#[ignore]reason provide clear follow-up tracking. No action needed.crates/warp-core/src/math/trig.rs (3)
80-101:sin_qtr_interpimplementation is sound.The defensive range check (line 84), endpoint handling (line 90-93), and linear interpolation are correctly implemented. The clippy allows are justified. No issues.
39-48: Symmetry enforcement is correct.The strategy of reducing
abs(angle)and applying the sign at the end guaranteessin(-x) = -sin(x)bit-for-bit. This addresses the determinism requirement for odd-function behavior. Well-documented comment.
20-28:canonicalize_zerois adequately documented.The doc comment clearly states the function's purpose. Since this is
pub(crate), full rustdoc isn't strictly required per coding guidelines, but it's appreciated.crates/warp-core/src/math/fixed_q32_32.rs (1)
1-189: Past review feedback addressed — conversion logic is sound.The excessive defensive programming flagged in the previous review has been properly addressed. The code now uses direct casts with
#[allow(clippy::cast_*)]attributes and justifying comments for conversions that cannot fail (lines 97-98, 119, 162-165, 183-186). The remainingtry_from()at line 68 is correctly retained sincei128toi64conversion is genuinely fallible.The Q32.32 conversion logic handles the full spectrum of f32 inputs deterministically: NaN→0, infinities saturate, subnormals/normals are processed with proper mantissa extraction and exponent scaling, and ties-to-even rounding is applied at boundaries. The inverse
to_f32path correctly canonicalizes-0.0and handles rounding overflow (line 178-180).crates/warp-core/src/payload.rs (6)
18-24: LGTM — Legacy type ID accessor properly documented.The v0 type ID accessor follows the established caching pattern and clearly documents the legacy 6×f32 format.
26-33: Updated to v2 canonical format — migration documented in PR objectives.The function now returns the v2 type ID. This is a breaking change from the previous v0 behavior, but aligns with the stated PR objective of making Q32.32 fixed-point the canonical format. Documentation correctly reflects the updated semantics.
59-70: LGTM — Direct Q32.32 encoder follows DRY principle.This encoder variant allows callers with pre-scaled Q32.32 integers to bypass the
from_f32conversion step. The implementation correctly mirrorsencode_motion_payload's byte layout.
138-157: Past review feedback properly addressed — dispatch limitation documented.The fragility concern from the previous review has been resolved with explicit documentation (lines 144-145) warning that this function dispatches solely by byte length and recommending type-aware decoding when possible. The dispatch order (v2 before v0) correctly prioritizes the canonical format.
159-188: Type-aware decoding properly handles both formats.Both functions correctly use
type_idfor unambiguous routing between v0 and v2 encodings, avoiding the length-based dispatch fragility. Thedecode_motion_atom_payload_q32_32documentation accurately describes the deterministic quantization path for legacy v0 payloads.
200-260: Test coverage validates both encoding paths and boundary conditions.The test suite correctly verifies:
- v0 round-trip with bit-exact comparison (appropriate for raw f32 encoding)
- v2 round-trip with value equality (appropriate for Q32.32 representation)
- Equivalence between
encode_motion_payloadandencode_motion_payload_q32_32for matching inputs- Rejection of invalid payload lengths around both format boundaries
crates/warp-core/src/math/scalar.rs (7)
17-44: Module documentation accurately scopes the current deliverables.The updated documentation correctly notes that feature gates exist but full engine-wide backend selection is follow-up work (lines 17-18), and the implementation note (lines 32-34) provides useful context about the LUT-backed trig backend. Imports are properly feature-gated.
193-206: F32Scalar trig correctly routes through deterministic LUT backend.The trig methods now invoke
trig::sin_cos_f32instead of raw libm functions, ensuring deterministic behavior across platforms. Each result is properly wrapped withSelf::new()to apply canonicalization. Thesin_cosmethod efficiently computes both values in a single backend call.
252-294: DFix64 struct documentation and accessors meet requirements.The struct documentation thoroughly explains the Q32.32 encoding and determinism contract (lines 252-265). Derived traits are appropriate for a deterministic numeric type. The
from_rawandrawaccessors are correctly markedconstand documented.
326-346: Multiplication logic is correct; defensive try_from is harmless but unreachable.The fixed-point multiplication correctly widens to
i128, descales by right-shifting 32 bits, and applies ties-to-even rounding.Line 338's
try_from(rounded).map_or(i128::MAX, |v| v)is overly defensive:roundedis derived fromabs >> 32whereabs ≤ 2^127, sorounded ≤ 2^95, which always fits ini128. The fallback branch is unreachable but harmless. Given this is internal saturating arithmetic, defensive code is arguably acceptable.
348-380: Past review feedback addressed — division logic with deterministic zero handling.The division-by-zero handling now includes the requested comment (line 351) explaining the
0/0 → 0policy for deterministic integer semantics, addressing the previous review. The division logic correctly scales the numerator, computes quotient/remainder, applies ties-to-even rounding, and handles sign via XOR.The
try_fromat line 372 is similarly defensive to the multiplication case; the fallback is likely unreachable but harmless in this saturating arithmetic context.
383-415: Scalar trait correctly implemented; trig precision limited by f32 backend.The
Scalartrait implementation forDFix64is correct. Arithmetic operations leverage the Q32.32 fixed-point representation, but trigonometric functions incur precision loss by round-tripping through f32 (lines 393-405). This is an intentional design choice: the LUT-backed trig backend operates on f32, and no higher-precision fixed-point trig implementation exists yet.This means DFix64's precision advantage applies only to arithmetic operations, not transcendentals. For the stated determinism objectives, this is acceptable — the results are deterministic across platforms even if not maximally precise.
417-460: Operator trait implementations correctly delegate to saturating arithmetic helpers.All five operator traits (
Add,Sub,Mul,Div,Neg) follow a consistent pattern: delegate to the corresponding saturating_rawhelper and wrap the result. This is clean, idiomatic, and correctly feature-gated.
|
Follow-ups after last update:
|
|
Addressed the latest CodeRabbit follow-ups (commit 30896c4):
|
|
Follow-up pushed to address remaining review nits + keep docs/issue tracking consistent:
Local verification:
CI was green before this push; waiting for the new run. When it’s done, would appreciate a re-review (reviewDecision currently shows CHANGES_REQUESTED). |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/src/payload.rs (1)
114-167: Consider#[inline]on internal decode helpers for consistency.The encode functions carry
#[inline], but these internal decode helpers do not. They're small, branch-heavy functions likely to sit on hot paths (motion rule execution). Adding#[inline]would give the compiler a hint and maintain stylistic consistency with the encode counterparts.
♻️ Duplicate comments (1)
docs/SPEC_DETERMINISTIC_MATH.md (1)
41-47: Markdown list indentation remains inconsistent — static analysis is correct.The linter flags MD005/MD007 because the nested list structure is malformed. The outer items use
*(asterisk + 3 spaces), while the inner items are indented with 2 spaces before the asterisk, creating inconsistent indentation levels.For compliant nesting under the "4-space after marker" parent style, the sub-items need matching alignment:
🔎 Proposed fix
* **Hardware Transcendentals:** `F32Scalar::sin/cos` previously delegated to `f32::sin/cos`. **Risk:** High (varies across libc/hardware implementations). - * *Status:* Implemented deterministic LUT-backed trig in `warp_core::math::trig` (Issue #107). + * *Status:* Implemented deterministic LUT-backed trig in `warp_core::math::trig` (Issue #107). * **Implicit Hardware Ops:** `Add`, `Sub`, `Mul`, `Div` rely on standard `f32` ops. - * *Risk:* Subnormal handling (DAZ/FTZ) depends on CPU flags. - * *Status:* `F32Scalar::new` flushes subnormals to `+0.0` at construction and after operations. + * *Risk:* Subnormal handling (DAZ/FTZ) depends on CPU flags. + * *Status:* `F32Scalar::new` flushes subnormals to `+0.0` at construction and after operations. * **NaN Propagation:** `f32` ops produce hardware-specific NaN payloads. - * *Status:* `F32Scalar::new` canonicalizes NaNs to `0x7fc0_0000`. + * *Status:* `F32Scalar::new` canonicalizes NaNs to `0x7fc0_0000`.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
crates/warp-core/src/bin/gen_sin_qtr_lut.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/lib.rscrates/warp-core/src/math/mat4.rscrates/warp-core/src/math/quat.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/math/trig.rscrates/warp-core/src/payload.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/tests/engine_motion_negative_tests.rsdocs/SPEC_DETERMINISTIC_MATH.mddocs/decision-log.mddocs/execution-plan.mdscripts/check_no_raw_trig.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/src/math/mat4.rscrates/warp-core/src/lib.rscrates/warp-core/src/math/quat.rscrates/warp-core/src/bin/gen_sin_qtr_lut.rscrates/warp-core/src/math/trig.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/src/math/scalar.rscrates/warp-core/tests/engine_motion_negative_tests.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/payload.rs
🧠 Learnings (4)
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Run `cargo clippy --all-targets -- -D missing_docs` before every PR; CI will expect a zero-warning, fully documented surface.
Applied to files:
scripts/check_no_raw_trig.shcrates/warp-core/src/demo/motion.rs
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
scripts/check_no_raw_trig.shcrates/warp-core/src/lib.rscrates/warp-core/tests/deterministic_sin_cos_tests.rscrates/warp-core/src/math/scalar.rscrates/warp-core/src/demo/motion.rscrates/warp-core/src/payload.rs
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: When a PR touches non-doc code, update `docs/execution-plan.md` and `docs/decision-log.md` in the same PR.
Applied to files:
docs/execution-plan.md
🧬 Code graph analysis (9)
crates/warp-core/src/math/mat4.rs (1)
crates/warp-core/src/math/trig.rs (2)
sin_cos_f32(36-80)canonicalize_zero(24-30)
crates/warp-core/src/lib.rs (1)
crates/warp-core/src/payload.rs (7)
encode_motion_atom_payload_v0(107-112)encode_motion_payload(54-61)encode_motion_payload_q32_32(83-89)encode_motion_payload_v0(70-76)encode_motion_payload_v0(231-237)motion_payload_type_id(31-33)motion_payload_type_id_v0(22-24)
crates/warp-core/src/math/quat.rs (1)
crates/warp-core/src/math/trig.rs (1)
sin_cos_f32(36-80)
crates/warp-core/src/math/trig.rs (1)
crates/warp-core/src/math/trig_lut.rs (1)
sin_qtr_sample(158-161)
crates/warp-core/tests/deterministic_sin_cos_tests.rs (1)
crates/warp-core/src/math/scalar.rs (1)
new(141-155)
crates/warp-core/src/math/scalar.rs (2)
crates/warp-core/src/math/trig.rs (1)
sin_cos_f32(36-80)crates/warp-core/src/math/fixed_q32_32.rs (3)
saturate_i128_to_i64(67-75)from_f32(83-141)to_f32(146-189)
crates/warp-core/tests/engine_motion_negative_tests.rs (1)
crates/warp-core/src/payload.rs (3)
decode_motion_atom_payload(195-203)encode_motion_atom_payload_v0(107-112)motion_payload_type_id(31-33)
crates/warp-core/src/demo/motion.rs (1)
crates/warp-core/src/payload.rs (8)
decode_motion_atom_payload(195-203)decode_motion_atom_payload_q32_32(211-219)encode_motion_payload_q32_32(83-89)motion_payload_type_id(31-33)encode_motion_payload(54-61)encode_motion_payload_v0(70-76)encode_motion_payload_v0(231-237)motion_payload_type_id_v0(22-24)
crates/warp-core/src/payload.rs (3)
crates/warp-core/src/attachment.rs (3)
new(144-146)new(257-262)new(305-307)crates/warp-core/src/ident.rs (1)
make_type_id(53-58)crates/warp-core/src/math/fixed_q32_32.rs (2)
from_f32(83-141)to_f32(146-189)
🪛 markdownlint-cli2 (0.18.1)
docs/SPEC_DETERMINISTIC_MATH.md
42-42: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (33)
crates/warp-core/src/math/quat.rs (1)
53-60: LGTM: Deterministic trig integration correct.The migration to
trig::sin_cos_f32eliminates platform-dependent libm calls, and the redundant canonicalization has been correctly removed. The values returned bysin_cos_f32are already canonical (pertrig.rsline 77), so direct assignment is appropriate.crates/warp-core/src/math/mat4.rs (1)
67-99: LGTM: Consistent deterministic trig migration across all axes.All three rotation functions (
rotation_x,rotation_y,rotation_z) correctly:
- Use
trig::sin_cos_f32to obtain canonicalized(s, c)pairs- Apply
canonicalize_zeroonly to the negation (-s → ns) to prevent-0.0contamination- Preserve correct matrix layouts per axis
The redundant canonicalization flagged in past reviews has been properly eliminated. The pattern repetition across three functions is acceptable—each axis requires a different matrix layout, so extraction would provide minimal benefit.
crates/warp-core/src/lib.rs (1)
90-94: All four newly exported payload functions carry rustdoc documentation—no violations.Inspection confirms:
motion_payload_type_id_v0()documentedencode_motion_payload_v0()documentedencode_motion_payload_q32_32()documentedencode_motion_atom_payload_v0()documentedThe crate adheres to the mandatory "every public API carries rustdoc" standard. Coding guidelines compliance verified. Clear to merge.
docs/execution-plan.md (2)
38-46: LGTM — Execution plan properly tracks PR #167 scope and validation.The entry correctly documents:
- Scope constraints (removing unrelated diff noise)
- Concrete exit criteria with exact commands
- Evidence with commit SHAs
The validation commands align with CI expectations. This is how execution plans should be maintained.
48-66: Completed items appropriately documented with evidence.Each completed milestone entry includes:
- Clear goal statements
- Evidence blocks with
cargo test/cargo clippyvalidation- Commit references for traceability
The progression from branch maintenance → deterministic trig → DFix64 → motion payload v2 shows logical milestone sequencing.
docs/SPEC_DETERMINISTIC_MATH.md (1)
56-83: Local validation section is excellent — mirrors CI exactly.This section provides developers with the exact commands CI runs, ensuring local/CI parity. The coverage of:
- Default
det_floatlane- Feature-gated
det_fixedlane- MUSL portability lane with install instructions
This is documentation done right. Future contributors will thank you.
crates/warp-core/tests/deterministic_sin_cos_tests.rs (5)
1-13: Headers and constants properly defined.SPDX header present. The bit-pattern constants for
ZERO,ONE, andCANON_NANare correctly defined and self-documenting. Using hex literals for IEEE 754 bit patterns is the right approach for determinism tests.
25-45: ULP difference calculation is correct and well-documented.The "ordered float" mapping (lines 36-42) correctly handles the sign-magnitude to two's-complement-like ordering required for ULP distance calculation. The NaN short-circuit returning
u32::MAXis the right sentinel.One pedantic observation: the comment says "negative floats map to the lower half" but the actual mapping inverts negative floats via
!bits, which maps them to the upper half of the u32 range (values starting with 0x7fff... after inversion). The math is correct; the comment could be more precise.
102-117: Index-based sampling eliminates cumulative float drift.Good fix from the previous review. Computing
angledirectly fromiavoids the error accumulation inherent in repeatedangle += step. The sampling range[-TAU, TAU]with 2049 samples provides sufficient coverage for canonicalization validation.
119-159: Golden vectors enforce bit-exact determinism across platforms.The vectors now demonstrate exact odd-symmetry for sine:
sin(π/8) = 0x3ec3_ef15sin(-π/8) = 0xbec3_ef15(exact sign-bit flip)This is the correct invariant for a deterministic sine implementation. The quadrant coverage (π/8, π/4, π/2, π, 3π/2, 2π) exercises all four quadrants of the LUT reconstruction.
161-199: Ignored test properly tracked with issue reference.The
#[ignore]annotation with reason string andTODO(#177)comment provides clear traceability. The test body is useful for manual error-budget audits while the deterministic oracle is pending.Note: Lines 182-183 use
+ 0.0to canonicalize signed zeros from the f64 reference — this is a nice touch to avoid false positives from-0.0comparisons.crates/warp-core/src/math/trig.rs (4)
1-20: Module header and constants properly structured.The module documentation clearly explains the strategy: range reduction → quarter-wave LUT → linear interpolation → quadrant symmetries. The
FRAC_3PI_2named constant improves readability over the inline3.0 * FRAC_PI_2expression.
22-30:canonicalize_zerois correct but could be micro-optimized.The equality check
value == 0.0matches both+0.0and-0.0per IEEE 754, returning canonical+0.0. This is correct.Pedantic alternative:
if value.to_bits() == 0x8000_0000 { 0.0 } else { value }would only touch-0.0specifically, avoiding the branch for non-zero values. However, the compiler likely optimizes this already, and clarity wins. Keeping as-is is fine.
36-80: Exact odd-symmetry enforcement is the critical correctness fix.The pattern of:
- Capture sign (line 49)
- Reduce
abs(angle)(line 50)- Apply sign at the end (lines 75-77)
ensures that
sin(-x)is the exact bitwise negation ofsin(x). This addresses the 1-ULP asymmetry issue from the previous review. The quadrant-based reconstruction (lines 54-73) using explicit comparisons rather thanfloor(r / FRAC_PI_2)is prudent — floating-point division can round up at boundaries.The comment on lines 52-53 explaining the rounding hazard is excellent documentation.
82-104: Linear interpolation is clean but the bounds check is defensive overkill.The
containscheck on line 86 should never trigger given the range reduction insin_cos_f32. However, defensive programming for future-proofing is acceptable.The
#[allow(...)]on lines 98-99 is justified:tis in[0, SIN_QTR_SEGMENTS_F32), so the cast tousizeis safe from truncation and sign loss.Minor style note: the early return on line 94 for
t >= SIN_QTR_SEGMENTS_F32handles the inclusivePI/2endpoint correctly.crates/warp-core/tests/engine_motion_negative_tests.rs (4)
5-18: Module documentation clearly explains the v0/v2 contract.The doc comment explains:
- v2 is canonical (Q32.32, 48 bytes)
- v0 is legacy-compatible for decode
- Upgrades happen on write
- Non-finite value mapping (NaN→0, ±∞→saturated)
This is exactly the level of detail needed for test documentation.
26-56: Test harness correctly validates upgrade-on-write semantics.The refactored
run_motion_once_with_payload:
- Accepts pre-constructed
AtomPayload(enabling v0 testing)- Asserts
ApplyResult::Applied(no silent failures)- Returns the post-execution
type_idfor upgrade verificationReturning the
type_idseparately is key — tests can now assert that v0 inputs produce v2 outputs.
81-93: v0→v2 upgrade test is the critical compatibility validation.This test proves legacy payloads work through the new executor:
- Input: v0 payload
- Output: v2 type_id
- Math: position updated correctly
Line 89 assertion on
motion_payload_type_id()verifies the upgrade occurred.
111-127: Infinity saturation constant is now documented.The
SATURATED_POS_F32constant with its comment (lines 119-122) explains the otherwise opaque2147483648.0value. This isi64::MAX >> 32asf32, which is the maximum representable value after Q32.32 saturation and round-trip throughto_f32.The test validates deterministic infinity handling — essential for cross-platform consistency.
crates/warp-core/src/demo/motion.rs (3)
40-76: Scalar backend abstraction is now properly documented.Each backend module has:
- Module-level doc explaining the strategy
- Per-function docs explaining the conversion semantics
The cfg-driven pattern cleanly separates:
det_fixed: Direct Q32.32 ↔DFix64(lossless)det_float: Q32.32 ↔ f32 ↔F32Scalar(lossy but deterministic)This addresses the previous review's documentation request.
87-124: v0 legacy support is now correctly implemented — critical issue resolved.The previous review flagged that
decode_motion_atom_payload_q32_32only handled v2. Per the relevant code snippets (payload.rs:210-218), this function now dispatches to:
decode_motion_payload_q32_32_v2for v2 payloadsdecode_motion_payload_q32_32_v0for v0 payloadsThis means legacy v0 payloads are deterministically quantized to Q32.32 at the decode boundary, then processed through the scalar backend, and always upgraded to v2 on write. The contract documented in
MOTION_RULE_NAMEis now honored.The comment on lines 88-90 correctly explains this dual-decode behavior.
270-304: New integration test proves v0→v2 upgrade path end-to-end.This test:
- Creates a v0 payload with
motion_payload_type_id_v0()- Runs the executor
- Asserts the output
type_idequalsmotion_payload_type_id()(v2)- Validates the math is correct
This is the definitive proof that legacy payloads work through the upgraded executor. The test belongs in the production code's test module rather than the integration test file since it exercises internal executor behavior directly.
crates/warp-core/src/math/scalar.rs (7)
31-44: Import structure cleanly separates deterministic backends.The unconditional
use crate::math::trig;import ensures bothF32ScalarandDFix64use the same deterministic trig backend. The conditionalfixed_q32_32import is correctly gated behinddet_fixed.
193-206: F32Scalar trig now uses deterministic LUT backend.All three trig methods delegate to
trig::sin_cos_f32, ensuring:
- Bit-exact results across platforms
- No reliance on platform libm
- Consistent canonicalization via
Self::new()The
sinandcosmethods callsin_cos_f32and discard one component. This is slightly wasteful but acceptable — the compiler may optimize, and correctness trumps micro-optimization here.
251-294:DFix64struct and basic methods are correctly defined.The Q32.32 encoding is clearly documented:
raw / 2^32 = real_valueZEROandONEconstants are correct (0and1 << 32)from_rawandraware simple accessorsThe
#[must_use]onfrom_rawis appropriate — discarding a constructed value is likely a bug.
322-342:mul_rawcorrectly implements round-to-nearest-ties-to-even.The algorithm:
- Compute full
i128product- Split into quotient and remainder after right-shift by
FRAC_BITS- Round up if remainder > half, or remainder == half AND quotient is odd
Lines 330-332 implement the ties-to-even logic correctly. The saturation on line 341 handles overflow gracefully.
344-376:div_rawhandles edge cases deterministically with documented 0/0→0 policy.The division-by-zero handling (lines 345-351) is now documented per the previous review. The choice of
0/0 → 0rather than NaN is unconventional but valid for integer-like semantics.The round-to-nearest-ties-to-even logic (lines 362-366) mirrors
mul_raw. The sign determination(a < 0) ^ (b < 0)on line 369 correctly handles mixed-sign division.
379-411:DFix64::sin/cosvia f32 round-trip is a pragmatic trade-off.The implementation converts to f32, computes trig via the deterministic LUT, then converts back to Q32.32. This introduces:
- Precision loss from Q32.32 → f32 (only ~23 bits of mantissa)
- Trig approximation error from the LUT
- Precision loss from f32 → Q32.32
For simulation purposes where sub-milliradian accuracy isn't critical, this is acceptable. The key property — determinism — is preserved because all operations are deterministic.
Verify trig precision is acceptable for your use case.
If high-precision fixed-point trig is later required, consider implementing a native Q32.32 CORDIC or polynomial approximation. The current f32 round-trip limits effective precision to ~23 significant bits.
296-304:saturate_i128_to_i64is now idiomatic.The
unwrap_or_elsepattern with the closure is cleaner than the previous match-with-allow. The saturation logic is correct: overflow clamps toi64::MINori64::MAXbased on sign.docs/decision-log.md (1)
9-13: LGTM — Deterministic math decisions are well-documented.The new entries clearly capture the rationale for v2 payloads,
DFix64, guardrails, and LUT-backed trig. The consequences section correctly notes the v0→v2 upgrade-on-write behavior and the tracking issue (#177) for the trig error-budget audit.crates/warp-core/src/payload.rs (3)
12-33: Clean type-id caching and versioning setup.The dual
OnceLockcaches for v0/v2 type IDs are thread-safe and avoid repeated hashing overhead. The label strings (payload/motion/v0,payload/motion/v2) clearly distinguish the wire formats. Good.
35-61: Encoding surface is well-documented and deterministic.The breaking-change callout (lines 37–39), non-finite input mapping (lines 50–52), and layout diagrams are exactly what downstream consumers need. The v0 encoder explicitly warns callers to prefer v2.
Also applies to: 63-89
169-219: Atom payload decoding routes correctly bytype_id.
decode_motion_atom_payloadanddecode_motion_atom_payload_q32_32dispatch ontype_idrather than byte length, which is the unambiguous path. The length-based dispatcher is appropriately caveated in its doc comment. This addresses the past review comment.
|
Addressed the newest open CodeRabbit “critical/major/minor” items on the PR head:
The motion executor still supports v0 + v2: |
|
Added a targeted regression test for the “executor breaks v0 support” concern:
This should make it unambiguous that the executor’s |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/warp-core/src/payload.rsscripts/check_no_raw_trig.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/src/payload.rs
🧠 Learnings (2)
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Run `cargo clippy --all-targets -- -D missing_docs` before every PR; CI will expect a zero-warning, fully documented surface.
Applied to files:
scripts/check_no_raw_trig.sh
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
scripts/check_no_raw_trig.shcrates/warp-core/src/payload.rs
🔇 Additional comments (1)
crates/warp-core/src/payload.rs (1)
12-302: Flawless execution of dual-encoding migration strategy.This implementation delivers:
- Deterministic v2 canonical format (Q32.32, 48 bytes) with non-finite sanitization via
fixed_q32_32::from_f32- Legacy v0 compatibility (f32, 24 bytes) with explicit upgrade path (
decode_motion_payload_q32_32_v0)- Unambiguous routing: length-based fallback (
decode_motion_payload) and type-id dispatch (decode_motion_atom_payload) with documented caveats- Complete API surface: separate encode/decode for v0/v2, raw Q32.32 variants, and atom payload wrappers
- Rigorous validation: all past review concerns addressed—
#[must_use]annotations, test shadowing eliminated, breaking changes documented, dispatch fragility notedTest coverage validates round-trips, Q32.32 equivalence, wrong-length rejection, and v0→Q32.32 quantization. Documentation meets the
missing_docsstandard. Zero correctness, safety, or style defects detected.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/check_no_raw_trig.sh
🔇 Additional comments (5)
scripts/check_no_raw_trig.sh (5)
1-5: LGTM: Strict mode and licensing in place.The shebang, license header, and
set -euo pipefailare correctly configured for a robust, portable bash script.
7-9: LGTM: Root discovery is correct.The script correctly resolves the repository root using
BASH_SOURCE[0]and changes to that directory. The quoting handles edge cases with spaces in paths.
11-18: LGTM: Policy is well-documented.The comment block clearly explains the determinism policy and the narrow enforcement scope. The target directory is correctly specified relative to the repository root.
20-24: LGTM: Directory guard correctly addresses past critical issue.The existence check fails fast with a clear error message if the target directory is missing, preventing the silent success scenario flagged in previous reviews.
38-39: No action needed. Thecrates/warp-core/src/mathdirectory has a flat structure with no subdirectories, making the basename exclusion approach correct. The excluded files (scalar.rs,trig.rs,trig_lut.rs) exist only at the top level where they're intended. The hypothetical over-exclusion scenario does not apply.Likely an incorrect or invalid review comment.
|
More review follow-ups landed on the latest head:
At this point, |
|
Merged
Local verification on merge commit
PR state:
|
Closes #165
Summary
This PR merges the deterministic-math milestone work that has been living on the
F32Scalar/sin-cosbranch.Highlights:
F32Scalar::{sin,cos,sin_cos}via checked-in LUT + linear interpolation..sin/.cos/.sin_cosinwarp-coremath outside the deterministic backend.det_fixedlane:DFix64(Q32.32) backend + unit tests + CI lanes (glibc/musl/macOS) for--features det_fixed.payload/motion/v2, 6×i64 LE).payload/motion/v0, 6×f32 LE).Scalarand upgrades v0→v2 on write.Determinism Notes
payload/motion/v2avoids NaN payload drift and subnormal/sign-zero surprises in attachment bytes.f32inputs are mapped deterministically at the fixed-point boundary:NaN→0+∞/-∞→ saturated extremaValidation
Pre-push hooks ran the full suite before pushing this branch:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warnings -D missing_docscargo test --workspaceAdditional spot checks used during development:
cargo test -p warp-core --features det_fixedcargo clippy -p warp-core --all-targets --features det_fixed -- -D warnings -D missing_docsCodeRabbit Review Loop (Required)
After CodeRabbit posts review comments, follow:
/Users/james/git/jitos/docs/procedures/EXTRACT-PR-COMMENTS.mdSuggested starting point:
gh pr view <PR_NUMBER> --json reviews --jq ...gh api repos/flyingrobots/echo/pulls/<PR_NUMBER>/comments > /tmp/pr-comments-...json/tmp/batch-N-issues.md