Skip to content

Reject raw-encode domains that collapse after f32 conversion#222

Closed
Alek99 wants to merge 2 commits into
feat/append-skip-unchangedfrom
alek/raw-wire-f32-guard
Closed

Reject raw-encode domains that collapse after f32 conversion#222
Alek99 wants to merge 2 commits into
feat/append-skip-unchangedfrom
alek/raw-wire-f32-guard

Conversation

@Alek99

@Alek99 Alek99 commented Jul 23, 2026

Copy link
Copy Markdown
Member

Review fixes for #187's _raw_wire_ok guard, split out of #221 when that PR retargeted to main (these fixes belong to the raw-encode code, which lives on this stack).

  • P1 — the guard accepted domains whose f64 span survives an f32 cast but whose endpoints collapse to the same f32 ([1e30, 1e30 + 1e20]): every shipped value rounds to f32(lo) and the shader maps them all to the domain floor. It also accepted spans whose f32 reciprocal overflows ([0, 1e-45]), sending Inf into the map uniform. Both now require float32(lo) < float32(hi) and a finite f32 inverse span, falling back to the f64-normalized unit encode; tests pin both cases.
  • P2 — the reciprocal bound is compared in f64 against np.finfo(np.float32).max, never cast through np.float32: the demoting cast warns on overflow for exactly the subnormal spans being rejected, and np.seterr(over="raise") setups would turn that into a FloatingPointError inside build_payload. The overflow test runs its probe under np.errstate(over="raise") to pin it.

Full tests/test_scatter.py passes (82); the fallback tests also pass with -W error::RuntimeWarning (no warnings emitted).

Alek99 added 2 commits July 22, 2026 18:34
_raw_wire_ok checked that the f64 span survives an f32 cast, but not that
the *endpoints* stay distinct in f32 or that the f32 reciprocal of the
f32 span is finite. [1e30, 1e30 + 1e20] passed while both endpoints (and
every shipped value) round to the same f32 — the shader would map all
values to the domain floor; [0, 1e-45] passed while its inverse span
overflows to Inf in the map uniform. Both now fall back to the unit
encode, which normalizes in f64 before the wire. Tests pin both cases.
…eview)

The overflow guard tested finiteness by casting the reciprocal through
np.float32, which emits an overflow warning for exactly the subnormal
spans it exists to reject — and under np.seterr(over="raise") that
warning becomes a FloatingPointError inside build_payload, turning a
valid extreme domain into a crash instead of a unit-encode fallback.
Decide by f64 comparison against np.finfo(np.float32).max instead; the
test now runs the probe under np.errstate(over="raise") to pin it.
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR tightens the raw continuous-channel encoding guard. The main changes are:

  • Reject domains whose endpoints collapse after f32 conversion.
  • Reject domains whose f32 inverse span would overflow.
  • Add fallback tests that raise on NumPy overflow warnings.

Confidence Score: 4/5

The raw-wire guard needs to reject domains whose mapping subtraction overflows in the shader.

Wide opposite-sign domains can pass the Python guard, and the same domains produce non-finite f32 arithmetic in the shader. The endpoint-collapse and reciprocal-overflow cases are otherwise covered.

python/xy/channels.py

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex ran the requested verification, but its local artifact references were not uploaded.
  • Before, the inverse-span regression in build_payload failed with a FloatingPointError: overflow encountered in cast.
  • After, the strict-warning selection passed both fallback cases without warnings or exceptions, and full tests/test_scatter.py validation completed in 0.58 seconds.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
python/xy/channels.py Adds endpoint and reciprocal checks, but still permits domains whose subtraction overflows in the f32 shader.
tests/test_scatter.py Adds focused tests for endpoint collapse and inverse-span overflow without covering wide opposite-sign domains.

Reviews (1): Last reviewed commit: "Compare the f32 reciprocal bound in f64,..." | Re-trigger Greptile

Comment thread python/xy/channels.py
Comment on lines +521 to +522
lo32, hi32 = float(np.float32(lo)), float(np.float32(hi))
return lo32 < hi32 and 1.0 / (hi32 - lo32) <= float(np.finfo(np.float32).max)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Shader Subtraction Can Overflow

A domain such as (-3e38, 3e38) passes this guard because both endpoints are finite f32 values and the reciprocal is checked from a Python-f64 difference. The shader then evaluates a_cval - lo in f32, where values near the upper endpoint overflow to Inf, producing an invalid color coordinate or a saturated size. Reject domains whose f32 mapping subtraction is not finite.

Suggested change
lo32, hi32 = float(np.float32(lo)), float(np.float32(hi))
return lo32 < hi32 and 1.0 / (hi32 - lo32) <= float(np.finfo(np.float32).max)
lo32, hi32 = float(np.float32(lo)), float(np.float32(hi))
span32 = hi32 - lo32
return lo32 < hi32 and span32 <= float(np.finfo(np.float32).max) and 1.0 / span32 <= float(
np.finfo(np.float32).max
)

T-Rex Ran code and verified through T-Rex

@Alek99

Alek99 commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

Folding back per review direction — keeping #221 as the single PR. These two fixes patch _raw_wire_ok, which only exists on the #186–188 stack (not on main, where #221 now bases), so they cannot ride in #221's diff. They remain on branch alek/raw-wire-f32-guard (commits 1daca36, acc16f4), ready to cherry-pick into the stack or onto main when the stack rolls up.

@Alek99 Alek99 closed this Jul 23, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 97 untouched benchmarks
⏩ 1 skipped benchmark1


Comparing alek/raw-wire-f32-guard (acc16f4) with feat/append-skip-unchanged (fdd7ffc)2

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

  2. No successful run was found on feat/append-skip-unchanged (e7d8a57) during the generation of this report, so 2c62821 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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