Fix ValueError from ULP-adjacent tied filtration values in computeReeb - #112
Open
ishikaghosh2201 wants to merge 1 commit into
Open
ishikaghosh2201 wants to merge 1 commit into
ishikaghosh2201 wants to merge 1 commit into
Conversation
ishikaghosh2201
requested review from
lizliz
and
a lite review from Copilot
September 18, 2026 15:52
This branch has not been deployed
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.
Description
Fixes a crash in
computeReebthat occurs when two distinct filtrationvalues are exactly 1 float64 ULP apart.
computeReeb's half-edgesentinel height,
(now_min + now_max) / 2, has no representable floatstrictly between two ULP-adjacent values, so the computed midpoint
rounds to exactly
now_max-- making the sentinel numericallyindistinguishable from a real Reeb node about to be created at that
height. This spuriously triggers
add_edge's equal-height "collapse"branch (intended only for genuine ties), deleting the real vertex
before a later step tries to connect to it, causing:
Fix:
snap chains of filtration values that are numerically
indistinguishable at float64 precision (
rtol=1e-9, atol=1e-12)into one canonical value, before grouping vertices into sweep levels,
using the same "does the midpoint round back to one of the endpoints"
test that exposes the bug. The snapped values are then used
consistently for both level grouping and simplex classification
(upper / horizontal / lower). The original
LowerStarobject (K)is never mutated.
Motivation and Context
This is a more general version of a previously-closed issue
(
computeReeb error, closed 2026-03-20), which fixed the case ofexact tied heights. That fix does not cover the case where two
values are merely float64-indistinguishable rather than literally
equal -- which happens naturally on symmetric meshes (e.g. an
icosphere), where vertices with the same true height are computed
along different subdivision paths and land 1 ULP apart due to
floating-point rounding, not due to any real geometric difference.
Fixes #111
How has this been tested?
python -m unittest discover -s tests -p 'test_*.py'-- all tests passing.
tests/test_computereeb.py:TestMergeCloseValues: direct unit tests on the new_merge_close_valueshelper -- confirms ULP-adjacent values aremerged, genuinely distinct values are left untouched, and a
3-value chain all snaps to one representative.
TestComputeReebTiedHeights: a minimal, hand-traceable 5-vertexcase with two ULP-adjacent heights no longer raises
ValueErrorand produces a valid tree (
edges == nodes - 1); a genuineexact-tie case (the originally-fixed bug) still works, guarding
against regression; the docstring example (no near-ties) produces
identical node/edge counts to pre-fix behavior.
TestComputeReebTiedHeightsRealMesh(skipped iftrimeshisn'tinstalled): reproduces the original real-world crash on an
icosphere (
subdivisions=2) and confirms it now succeeds with81 nodes / 80 edges, a valid tree.
R.f, node set, edge set)between the patched and unpatched
computeReebon non-degeneratecases (docstring example,
subdivisions=1icosphere) -- confirmingthis fix is a no-op everywhere the bug doesn't apply.
subdivisions=2icosphere, only 2 of 42 unique vertex heights sitwithin the merge tolerance of each other; the next-smallest gap in
the dataset is ~9 orders of magnitude larger, so there's no
ambiguous middle ground being swept in by this tolerance choice.
Types of changes
Checklist
pyproject.tomlfile if a new version needs to be pushed to pypi. Note that if the number isn't incremented, the package will not be pushed to pypi, which is useful if this PR is only for updating documentation.make formatto clean up the code withblack.make tests).