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

Revert "Merge pull request #104 from ulupo/coo_format_patch" #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ulupo
Copy link
Contributor

@ulupo ulupo commented Nov 3, 2020

This reverts commit 07adc60.

The up-to-date version of upstream ripser which was integrated in the project by @MonkeyBreaker in #106 does not seem to suffer from the issues pointed out in #103 and addressed by #104.

It seems that there are no longer wrong outputs when sparse matrices are initialised in non-lexicographic ways. The unit test introduced by @ctralie in 1b565cf now serves as a regression test.

It would be nice to have @ubauer's or @MonkeyBreaker's confirmation that my reading of the situation is correct.

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #112 into master will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   96.75%   96.64%   -0.11%     
==========================================
  Files           3        3              
  Lines         154      149       -5     
  Branches       26       25       -1     
==========================================
- Hits          149      144       -5     
  Misses          4        4              
  Partials        1        1              
Impacted Files Coverage Δ
ripser/ripser.py 96.57% <100.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f784e1f...aa8b42c. Read the comment docs.

@MonkeyBreaker
Copy link
Contributor

Hi !

As far as I could test with the new backend, the issue described #103 doesn't seem to appear.
One thing that's is maybe a bit late, would be to add a test case where the issue was encountered before merging #106.
If we add a test case, then it would be perfect in my opinion.

Best,
Julián

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