Move hera to submodule - Githubissues

giotto-ai / giotto-tda

A high-performance topological machine learning toolbox in Python

https://giotto-ai.github.io/gtda-docs

Other

858 stars 175 forks source link

Move hera to submodule #371

Closed reds-heig closed 4 years ago

reds-heig commented 4 years ago

Reference issues/PRs

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)

Description

Previously hera submodule (bottleneck and wasserstein implementations) was not a submodule but sources were hard-coded. This PR removes old hera sources with new up-to-date repository maintained from the author itself.

Screenshots (if appropriate)

Any other comments?

Checklist

[x] I have read the guidelines for contributing.
[x] My code follows the code style of this project. I used flake8 to check my Python changes.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed. I used pytest to check this on Python tests.

CLAassistant commented 4 years ago

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

MonkeyBreaker commented 4 years ago

After looking into the issue of why pipelines are currently failing, I stumbled onto a similar issue reported recently to azure-pipelines.

The problem is that the copy step fails when encountering some symlinks, unfortunately, introducing hera repository, makes this issue appear, for the following files:

gtda/externals/hera/matching/tests/test_bifiltration_1.txt
gtda/externals/hera/matching/tests/test_bifiltration_full_triangle_rene.txt

I'll contact the maintainer of hera to fix this issue, but at the meantime we have 2 solutions:

wait till the maintainer fixes this issue
As recommended in issue , using a simple bash script to copy the file instead

MonkeyBreaker commented 4 years ago

After looking into the issue of why pipelines are currently failing, I stumbled onto a similar issue reported recently to azure-pipelines.

The problem is that the copy step fails when encountering some symlinks, unfortunately, introducing hera repository, makes this issue appear, for the following files:
* `gtda/externals/hera/matching/tests/test_bifiltration_1.txt`

* `gtda/externals/hera/matching/tests/test_bifiltration_full_triangle_rene.txt `
I'll contact the maintainer of hera to fix this issue, but at the meantime we have 2 solutions:
* wait till the maintainer fixes this issue

* As recommended in [issue ](https://github.com/Microsoft/azure-pipelines-tasks/issues/9046), using a simple bash script to copy the file instead

This has now been resolved

ulupo commented 4 years ago

@MonkeyBreaker thanks for this, and for the new tests!

MonkeyBreaker commented 4 years ago

I'll perform the changes this afternoon, I'm currently busy on another topic, but it'll be resolved today.