test(simd): add tolerance package for approximate equality; update SIMD tests#107
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
kolkov
left a comment
There was a problem hiding this comment.
Clean, well-structured contribution. The tolerance package is exactly what we needed (#92), and the Burn-inspired design is the right reference. Test coverage is thorough — edge cases, NaN/Inf, slice operations, all three modes. The new MinFloat/Large boundary tests are excellent additions.
Several items to address before merge:
checkRelAbs formula diverges from Burn's implementation
The checkRelAbs function uses |a+b| (absolute value of the sum):
relTol := float64(rel) * math.Abs(float64(a+b))Burn's actual approx_eq implementation (compare.rs:186-188) uses max(|a|, |b|):
let max = F::max(x.abs(), y.abs());
diff < self.absolute.max(self.relative * max)Note: Burn's struct-level doc comment (line 14) says |x + y|, but the actual code uses max(|x|, |y|). This is a doc bug in Burn — the relative() method doc (line 84) correctly says max(|x|, |y|). The code is the truth.
This formula difference has two practical effects:
Same-sign values (our SIMD test case): |a+b| ≈ 2 * max(|a|, |b|), making the PR ~2x more lenient than Burn. Example: a=100, b=99 with rel=0.01:
- Burn:
tol = max(abs, 0.01 * 100) = 1.0→diff=1.0, FAIL (strict<) - PR:
tol = max(abs, 0.01 * 199) = 1.99→diff=1.0, PASS
Since SIMD tests compare scalar vs SIMD on the same data, all values are same-sign. The extra leniency could mask real precision regressions.
Opposite-sign values: |a+b| → 0, collapsing the relative term entirely. Falls back to absolute tolerance only.
Suggested fix:
relTol := float64(rel) * math.Max(math.Abs(float64(a)), math.Abs(float64(b)))This also makes checkRelAbs consistent with checkRel, which already uses max(|a|, |b|).
Docstring inconsistency
The AssertApproxEqual docstring has the same inconsistency as Burn's docs — Rel says max(|a|, |b|) but RelAbs says |a+b|. After fixing the formula, update the RelAbs docstring and checkRelAbs comment to match.
NaN/Inf handling differs from Burn (document the choice)
The docstring says "Fails if either value is NaN." Burn treats NaN==NaN as equal and ±Inf==±Inf as equal. Your stricter approach is defensible (IEEE 754 purist), but since the PR body references Burn alignment, a brief comment noting this as an intentional divergence would help future readers.
Also note: +Inf - (+Inf) = NaN, so two equal infinities will fail — which may surprise callers.
Minor suggestions (not blockers)
-
No input validation — Burn asserts
relative <= 1.0andabsolute >= 0.0at construction. A negativeAbswould cause every comparison to silently fail. Consider documenting the constraint or adding a check. -
tolallocation in loop —NewDefaultTolerance[float32]()is called inside the subtest loop in every SIMD test. Since it returns the same values each time, hoisting it before the loop is cleaner. Zero functional impact, just style.
Overall: solid contribution. The checkRelAbs formula is the only required fix — switching from |a+b| to max(|a|,|b|) aligns with Burn's actual behavior and avoids ~2x leniency in our primary use case. The rest are suggestions. Looking forward to this landing.
|
Thanks for the feedback @kolkov!
|
kolkov
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough fixes @bennibbelink! All four items addressed cleanly. One remaining issue in checkRelAbs:
relTol := float64(rel) * math.Max(float64(a), float64(b))This is missing math.Abs — should be:
relTol := float64(rel) * math.Max(math.Abs(float64(a)), math.Abs(float64(b)))Same pattern as checkRel (which is correct) and Burn's F::max(x.abs(), y.abs()).
Without math.Abs, negative values produce a negative relTol, collapsing the relative component entirely. Example: a=-100, b=-100.04 with rel=0.01:
- Current:
max(-100, -100.04) = -100→relTol = -1.0→tol = max(1e-5, -1.0) = 1e-5→ diff0.04FAILS - Fixed:
max(100, 100.04) = 100.04→relTol = 1.0→tol = 1.0→ diff0.04PASSES
The existing RelAbs tests all use positive values so the bug isn't caught — consider adding a negative-values case to the TestAssertApproxEqual_RelAbs table.
kolkov
left a comment
There was a problem hiding this comment.
All review items addressed. checkRelAbs formula now matches Burn and checkRel, negative test cases added, input validation in place, NaN/Inf handling aligned with Burn. Clean work — thanks @bennibbelink!
Will merge once CI is fully green.
docs: add PR #107 tolerance package to CHANGELOG
Addresses #92
This PR introduces a reusable internal/tolerance package for approximate floating-point comparisons and migrates all SIMD arithmetic correctness tests to use it.
The design is inspired by Burn's Tolerance type:
The default values configured in
tolerance.NewDefaultToleranceare also inspired by Burn (see documentation link): "Another common initialization isTolerance::<F>::rel_abs(1e-4, 1e-5).set_half_precision_relative(1e-2)"Aside from a review of correctness, I'm keen to hear feedback on the API and ergonomics of the tolerance package introduced