giotto-ai / giotto-tda

A high-performance topological machine learning toolbox in Python
https://giotto-ai.github.io/gtda-docs
Other
845 stars 173 forks source link

Replace ripser.py and robinhood submodules with giotto-ph dependency #614

Closed ulupo closed 2 years ago

ulupo commented 2 years ago

Following the release of giotto-ph v0.2.1, we should be able to confidently remove the ripser.py and robinhood submodules, as well as the bindings for GUDHI's edge collapser, and add instead giotto-ph as a Python dependency.

@MonkeyBreaker this might be a good time to double-check that all tests in test_ripser.py and test_collapser.py have been migrated to the giotto-ph project.

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

ulupo commented 2 years ago

@MonkeyBreaker is the eigen git submodule still needed? I have tried to remove it in 6bd5a7c in a naive way, but this breaks the simplex tree interface from GUDHI.

MonkeyBreaker commented 2 years ago

@MonkeyBreaker is the eigen git submodule still needed? I have tried to remove it in 6bd5a7c in a naive way, but this breaks the simplex tree interface from GUDHI.

@ulupo looking were it is used in our fork of gudhi-devel, Eigen is used in the following places:

It seems that unfortunately we cannot remove for the moment eigen. And also, it seems that from looking at the simplex_tree, it needs the Flag_complex_edge_collapser.h file to compile. So we cannot not remove from our gudhi-devel the collapser.

Otherwise I clone this fork on my machine and I was able to compile, and run all the test.

ulupo commented 2 years ago

Hey @MonkeyBreaker, thanks! I'm ok to leave eigen for now. Perhaps there can be another PR which also brings our fork of gudhi-devel more up-to-date with the main branch there. I'll let you complete the review for this PR and just to reiterate:

@MonkeyBreaker this might be a good time to double-check that all tests in test_ripser.py and test_collapser.py have been migrated to the giotto-ph project.

MonkeyBreaker commented 2 years ago

@MonkeyBreaker this might be a good time to double-check that all tests in test_ripser.py and test_collapser.py have been migrated to the giotto-ph project.

Concerning test_ripser, all test from gtda are in gph. But, concerning test_collapser, gtda has additional test for csr format, this was removed because redundant with coo support.

In summary, all test for ripser and collapser from gtda were migrated in gph.

Thank you @ulupo for your effort on migrating gtda to use gph !

MonkeyBreaker commented 2 years ago

Hey @MonkeyBreaker, thanks! I'm ok to leave eigen for now. Perhaps there can be another PR which also brings our fork of gudhi-devel more up-to-date with the main branch there.

Definitely, this unfortunately takes some time ... and could break things if gudhi added additional dependencies in the modules we already integrated.

ulupo commented 2 years ago

In summary, all test for ripser and collapser from gtda were migrated in gph.

Just what I wanted to hear! Ok, I'll wait for your formal approval and for the CI to pass before merging.

MonkeyBreaker commented 2 years ago

Looking at the rest of the library the mention of ripser are in :

In the example file you mention, both libraries, this is perfectly fine. In the utils, ripser is when we want a vr filtration. In simplicial, it is in reference when we use ripser for computing a vr filtration.

For 2 last files, this is internal behavior to the library and users are not exposed to this.

LGTM !

Concerning the CI, we need to move to Github Action to be more confident on what is going on.

ulupo commented 2 years ago

The Install Boost step is failing because of a (504) Gateway Timeout error... Maybe the Boost website is down?

MonkeyBreaker commented 2 years ago

The Install Boost step is failing because of a (504) Gateway Timeout error... Maybe the Boost website is down?

Weird because for others windows jobs it works. Maybe the environment where things run have some issues ?

ulupo commented 2 years ago

for others windows jobs it works

But the other jobs were launched a few days ago. I'll re-run everything.

ulupo commented 2 years ago

/azp run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).