Skip to content

fix: decide constant folding structurally instead of by sampling (spec 07) - #25

Merged
minikin merged 1 commit into
mainfrom
spec/07-sound-constant-folding
Jul 15, 2026
Merged

minikin merged 1 commit into
mainfrom
spec/07-sound-constant-folding

Conversation

@minikin

@minikin minikin commented Jul 15, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • GraphOptimizer::is_constant/is_constant_zero/is_constant_one previously decided constancy by sampling a node 3-4 times and comparing results — a low-entropy distribution (e.g. bernoulli(0.99)) had a high chance of sampling the same value repeatedly by chance and being silently folded into a constant, changing the distribution the optimized graph actually represents.
  • ComputationNode::Leaf now carries constant_value: Option<T>, set only by the new ComputationNode::constant(value) constructor (and Uncertain::point, internally) or by the optimizer's own folded results. Every constancy check and constant-folding match arm reads this field directly — zero sample() calls anywhere in the decision path, matching the spec's own grep acceptance test literally.
  • Discovered along the way (documented in the spec's implementation notes): GraphOptimizer isn't wired into Uncertain<T>'s arithmetic operators at all — it's an opt-in utility invoked manually. Unchanged by this spec, flagged for Spec 08 (effective CSE has the same characteristic).
  • New acceptance-level tests: a deterministic low-entropy-distribution test (the bernoulli(0.99) scenario, now a hard guarantee rather than statistical), point(0.0)+x/point(2.0)*point(3.0) folding through the real public API, normal(0, 1e-12) never folded, and a same-seed optimized-vs-unoptimized equivalence test (sample_with + ChaCha8Rng).
  • Updated examples/optimizations/graph_optimization.rs's literal 0/1 demo operands to use ComputationNode::constant — with the old sampling-based check removed, the demo would otherwise silently stop demonstrating identity elimination/constant folding while still printing the same correct final number.
  • Not a breaking change — public API only gains ComputationNode::constant; Uncertain::point's signature/behavior is unchanged.

Test plan

  • just dev green (fmt, clippy -D warnings, tests default + parallel, doc tests, audit, deny, crap regression gate)
  • Full test suite + doctests pass with --all-features and --features parallel
  • Verified via awk/grep that no sample() call appears in is_constant_zero/is_constant_one's bodies
  • Ran examples/optimizations/graph_optimization.rs manually — still prints the correct 32.0 result both optimized and unoptimized
  • crap_baseline.json regenerated (10 functions improved from simplified call sites, 2 now-unused helpers removed)
  • CHANGELOG.md/specs/README.md updated

…c 07)

GraphOptimizer's is_constant/is_constant_zero/is_constant_one previously
sampled a node 3-4 times and compared the results to decide constancy.
A low-entropy distribution (e.g. bernoulli(0.99)) had a high chance of
sampling the same value repeatedly by chance and being silently folded
into a constant, changing the distribution the optimized graph represents.

ComputationNode::Leaf now carries a constant_value: Option<T>, set only
by the new ComputationNode::constant constructor (and Uncertain::point,
internally) or by the optimizer's own folded results. Every is_constant*
check and constant-folding match arm reads this field directly instead
of ever calling sample(), so constancy is now a structural fact about
how a node was built rather than an inference from observed behavior.
@minikin
minikin merged commit 08732c4 into main Jul 15, 2026
8 of 9 checks passed
@minikin
minikin deleted the spec/07-sound-constant-folding branch July 15, 2026 15:04
@minikin
minikin restored the spec/07-sound-constant-folding branch July 16, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant