Use f32 everywhere - #459
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Standardizes evaluation-facing numeric types on f32 across Fidget.
Changes:
- Migrates contexts, trees, shapes, transforms, and Rhai conversions to
f32. - Aligns evaluator tests with canonical
f32operations. - Routes JIT modulo operations through
rem_euclid.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the numeric migration. |
fidget-core/src/context/mod.rs |
Migrates context APIs and evaluation. |
fidget-core/src/context/op.rs |
Migrates opcode evaluation. |
fidget-core/src/context/tree.rs |
Migrates tree constants and transforms. |
fidget-core/src/eval/test/float_slice.rs |
Tightens float-slice comparisons. |
fidget-core/src/eval/test/grad_slice.rs |
Updates gradient testing for f32. |
fidget-core/src/eval/test/interval.rs |
Updates interval comparisons. |
fidget-core/src/eval/test/mod.rs |
Simplifies canonical operations. |
fidget-core/src/eval/test/point.rs |
Tightens point comparisons. |
fidget-core/src/types/float.rs |
Adds canonical logical-not behavior. |
fidget-jit/src/aarch64/float_slice.rs |
Uses baseline modulo helper. |
fidget-jit/src/aarch64/point.rs |
Uses baseline modulo helper. |
fidget-jit/src/x86_64/float_slice.rs |
Uses baseline modulo helper. |
fidget-jit/src/x86_64/point.rs |
Uses baseline modulo helper. |
fidget-mesh/src/octree.rs |
Removes obsolete numeric casts. |
fidget-rhai/src/lib.rs |
Adds f32 dynamic conversion. |
fidget-rhai/src/shapes.rs |
Notes eager default construction. |
fidget-rhai/src/tree.rs |
Converts Rhai scalars to f32. |
fidget-rhai/src/types.rs |
Converts Rhai vectors and planes. |
fidget-shapes/src/lib.rs |
Migrates shape parameters and transforms. |
fidget-shapes/src/types.rs |
Migrates shape value types. |
fidget/tests/pixel_render.rs |
Updates infinity constant type. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
586
to
+588
| (out == value) | ||
| || C::discontinuous_at(lhs, rhs) | ||
| || err < 1e-6 | ||
| || value.is_nan() && out.is_nan(), | ||
| "mismatch in '{name}' at ({lhs}, {rhs}): \ | ||
| {value} != {out} ({err})" | ||
| || value.is_nan() && out.is_nan() | ||
| || value.is_nan() && constant_folded, |
Comment on lines
425
to
+427
| (o == v) | ||
| || C::discontinuous_at(*a, *b) | ||
| || err < 1e-6 | ||
| || (v.is_nan() && o.is_nan()), | ||
| "mismatch in '{name}' at {a} {b}: {v} != {o} ({err})" | ||
| || (v.is_nan() && o.is_nan()) | ||
| || (v.is_nan() && constant_folded), |
Comment on lines
1153
to
+1155
| if inside_value.is_nan() || inside_value.is_infinite() { | ||
| assert!( | ||
| out.has_nan(), | ||
| out.has_nan() || constant_folded, |
Comment on lines
+568
to
+570
| o.v == v | ||
| || (v.is_nan() && o.v.is_nan()) | ||
| || (v.is_nan() && constant_folded), |
mkeeter
force-pushed
the
f32-everywhere
branch
from
August 18, 2026 23:49
dc3a55b to
d0f9bb4
Compare
mkeeter
force-pushed
the
f32-everywhere
branch
from
August 18, 2026 23:50
d0f9bb4 to
c597f10
Compare
Owner
Author
|
I'm disregarding the Copilot comments about the constant-folded + NaN case; constant folding is allowed to not be NaN-preserving. |
virtualritz
pushed a commit
to virtualritz/fidget-koto
that referenced
this pull request
Aug 19, 2026
Upstream fidget moved shapes fields, Tree::constant, and the Vec types from f64 to f32 (mkeeter/fidget#459, rev cba86d1). Narrow at the fidget boundary -- script-facing signatures keep f64, the casts happen where Tree and shape structs are populated -- and relock the fidget family to the f32 rev. Also repair two pre-existing test failures: doctests using the removed fidget::Error type, and a koto script using the retired inline 'export name: value' form. Verified: cargo check/test/clippy --workspace clean (10 unit tests, 2 doctests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdrNsaPqhhY9V7Awp17wEf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fidget's evaluators have always used
f32values. However, many other parts of the pipeline (constants in aContext, parameters in a shape, transform matrices, etc) usedf64s. This was particularly noticeable in evaluation: the context's tree-walk evaluator usedf64values, as did the evaluator test suite. Tiny discrepancies necessitated a bunch of extra logic (epsilons everywhere, etc).This PR switches to using
f32everywhere in Fidget (for things that eventually end up being evaluated). It then deletes and simplifies a bunch of awkward hacks and workarounds.Notably, the evaluator test suite now requires are in exact agreement with the canonical implementation. Unfortunately, the assembly implementations of
modulofell by the wayside; they now call into the baseline implementation, at slightly higher overhead.