Repository navigation
fix: fingerprint quantization no longer splits a value on a boundary (#87) - #90
Merged
Merged
Conversation
…t sits on a boundary (#87) DECISION: snap the ratio value/tolerance to a binary grid before rounding it to an integer. Do not pre-round to a fixed number of decimals, do not make the snapshot validator tolerant of off-by-one integers. PROBLEM A body's geometry fingerprint is a hash of every measured value divided by 1e-7 mm and rounded to an integer (`_q`, artifacts/geometry_interface.py). A design value such as -3.34690815 mm is exactly a boundary of that rounding, (k + 0.5) * 1e-7. The value computed one way is a unit in the last place under the boundary, the same value after a BREP round trip is one over it (-3.3469081499999995 against -3.34690815), and `int(round(...))` sends the two to neighbouring integers. The reporter's body had two such edge centers: its fingerprint changed across the readback and `restore_topology_snapshot` raised `geometry_mismatch`. REPRODUCED - On the attachment of the issue (R02 BREP + recorded snapshot, read in this checkout, without running the attachment's own script): the fingerprint after the readback is exactly the issue's B (sha256:d9620b...), and the two edge centers quantize to -33469082 and -66530918, the integers the issue lists after the readback. - A whole solid that changes its fingerprint across a BREP round trip could not be built synthetically: the doubles survive the binary BREP unchanged, the one-ulp difference of the issue comes from how the readback shape is measured (centers are integrated). So the regression test is at the quantizer, with the issue's two value pairs and 20000 random values on a boundary with up to three ulps of noise on each side: before this change 6482 of them split, and both issue pairs failed. WHY THIS FIX - Pre-rounding to a fixed number of decimals either leaves large coordinates (a 1 m part is 1e10 quanta, one ulp is 2e-6 of a quantum) unprotected or moves 1% of all values to a different integer. The grid follows the ratio's magnitude (2**-46 of it, at least 2**-24): a half-integer is exactly on it, noise is well under it, and the window of values that change integer is about 1e-5 of a quantum (test: at most 5 of 50000 ordinary values move). - A tolerant validator (accept an off-by-one integer) would have to be taught every place a label is compared (body, face, edge and vertex labels, the topology match keys, the connector hashes); one deterministic quantizer fixes them all at the source. WHAT THIS DOES NOT FIX - Fingerprints are stored in artifacts. A value that was on a boundary now quantizes (half to even) where it used to fall on either side, so such a stored fingerprint changes once; every other value keeps its integer. The reporter's recorded snapshot (made on the other side of the boundary) still does not restore: it has to be captured again. It could not have been restored before either. - `exporter/_surface_mesh.py` and `inspect/brep/compare.py` round the same way for mesh welding and BREP comparison; they were not part of the report and are left alone. Checked: test_artifact_*, test_product_*, test_brep_region_snapshot, test_topology_getter_contract, test_part_interface, test_assembly_builder, test_native_canonical pass. test_scene_contract (generated types) and test_native_without_ocp fail on the base commit as well (this machine lacks OCP and the pinned generator). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
Fixes #87.
A design value such as
-3.34690815mm lies exactly on a boundary of theround(value / 1e-7)quantizer behind geometry fingerprints; computation noise of one ulp (the readback of a BREP) sends it to the neighbouring integer, so the fingerprint changed andrestore_topology_snapshotraisedgeometry_mismatch._qnow snaps the ratio to a magnitude-scaled binary grid before rounding.Reproduced on the issue's attachment (without running its script): the readback fingerprint equals the issue's B and the two edge centers quantize to the integers it lists. The regression test is at the quantizer (the issue's value pairs + 20000 random boundary values with ulp noise: 6482 split before, 0 now). A whole-solid synthetic repro was not possible, see the commit message.
Compatibility: a stored fingerprint of a body with a value on a boundary changes once; the reporter's recorded snapshot has to be captured again. The commit message has the decision record.
🤖 Generated with Claude Code