Skip to content

Fix bottleneck returning NaN and wasserstein raising for -inf births - #114

Open
Yuren0312 wants to merge 1 commit into
scikit-tda:masterfrom
Yuren0312:fix-inf-birth-nan
Open

Yuren0312 wants to merge 1 commit into
scikit-tda:masterfrom
Yuren0312:fix-inf-birth-nan

Conversation

@Yuren0312

Copy link
Copy Markdown

Fixes #111.

Problem

bottleneck and wasserstein only filter non-finite death times, never birth times. A diagram containing a point with a -inf birth (which can arise, e.g. from ripser's lower_star_img when the image holds -inf anchor pixels, or from a negated superlevel persistence) therefore leaks -inf into the cross-similarity matrix:

  • In bottleneck, -inf - (-inf) evaluates to NaN, which lands in D and becomes ds[-1], so the returned distance is silently NaN.
  • In wasserstein, pairwise_distances produces NaN for the -inf coordinate, and scipy.optimize.linear_sum_assignment raises ValueError.

Fix

Filter on both coordinates — np.isfinite(S[:, 0:2]).all(axis=1) — so a non-finite birth is handled the same way a non-finite death already is: the point is dropped with a UserWarning, consistent with the existing convention. The warning text is updated to "non-finite birth or death times" (the change is reflected in the four existing test_inf_deathtime matches).

Tests

  • Updated the warning matches in the existing test_inf_deathtime tests (bottleneck + wasserstein).
  • Added test_inf_birthtime to both TestBottleneck and TestWasserstein, covering a [-inf, 0.5] point and asserting a finite result (the exact 0.5 bottleneck distance, and a finite symmetric wasserstein distance).

Verification

  • pytest (full suite) → 114 passed
  • ruff check persim/ and ruff format --check persim/ → clean

This branch has not been deployed

No deployments
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.

Bottleneck returns NaN and Wasserstein raises for -inf births

1 participant