Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Computing distances between two empty PersistenceDiagrams #54

Merged
merged 5 commits into from
Nov 26, 2022

Conversation

mbudisic
Copy link
Contributor

A distance (either Wasserstein or Bottleneck) between two empty PersistenceDiagrams should return zero. (Empty diagrams contain trivially the "diagonal" in the birth-death graph, and distance between the diagram and itself should be zero.)

Before this change, the code produces an error when applying either hungarian or _hopcroft_karp algorithms in matching.jl, and the two errors are different.

The pull request performs a simple test at the beginning of Bottleneck() and Wasserstein() functions and returns the distance zero or a zero Matching.

The pull additionally adds a unit test group for this issue.

@coveralls
Copy link

coveralls commented Nov 26, 2022

Pull Request Test Coverage Report for Build 3552291527

  • 6 of 8 (75.0%) changed or added relevant lines in 1 file are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.9%) to 94.954%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/matching.jl 6 8 75.0%
Files with Coverage Reduction New Missed Lines %
src/matching.jl 16 91.88%
Totals Coverage Status
Change from base Build 2382344384: -1.9%
Covered Lines: 828
Relevant Lines: 872

💛 - Coveralls

Copy link
Owner

@mtsch mtsch left a comment

Choose a reason for hiding this comment

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

Thanks so much, I can't believe I missed such an obvious edge case!

Looks good, but I'd rather it returns a 0.0, so the distances are always Float64s.

src/matching.jl Outdated Show resolved Hide resolved
src/matching.jl Outdated Show resolved Hide resolved
test/matching.jl Outdated Show resolved Hide resolved
mbudisic and others added 3 commits November 25, 2022 23:56
Co-authored-by: mtsch <matijacufar@gmail.com>
Co-authored-by: mtsch <matijacufar@gmail.com>
Co-authored-by: mtsch <matijacufar@gmail.com>
@mbudisic
Copy link
Contributor Author

Ah yes - I didn't pay attention to this. I accepted the suggestions.

Thanks for developing the package, my collaborators and I are enjoying using it!

Copy link
Owner

@mtsch mtsch left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good now. I'll bump the version and merge/register it now.

@mtsch mtsch merged commit 73ba367 into mtsch:master Nov 26, 2022
@mtsch
Copy link
Owner

mtsch commented Nov 26, 2022

The fix is registered and can now be installed with the package manager. Thanks again!

@mbudisic
Copy link
Contributor Author

My pleasure!

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.

3 participants