Fix #499 by covering list input in CubicalPersistence

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

Fix #499 by covering list input in CubicalPersistence #503

Closed ulupo closed 4 years ago

ulupo commented 4 years ago

Reference issues/PRs Fixes #499.

Types of changes

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

Description To avoid #499, ndarray and list input are handled separately in CubicalPersistence.fit for the purpose of finding maximum pixel values.

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.
[x] My change requires a change to the documentation.
[x] 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.

gtauzin commented 4 years ago

If we are going to change that behavior in the short term, I would just avoid having 2 breaking changes in a row and just go for the final result instead.

ulupo commented 4 years ago

If we are going to change that behavior in the short term, I would just avoid having 2 breaking changes in a row and just go for the final result instead.

So is your preferred resolution of the bug we have to adapt the maximum calculation to the list scenario?

gtauzin commented 4 years ago

Either that or maybe just wait for the final fix. I think it's a niche bug as it's unlikely that anyone will use the transformer of list of images of different shapes in the near future.

Actually, none of the images transformer can actually deal with images of different shapes and this is not going to change in the future. I am really not sure there is a need for that capability in the first place. xD

ulupo commented 4 years ago

@gtauzin thanks! The capability would be for time series of variable length for the most part, I think (since then one has the sublevel set filtration). But I agree that it is a niche bug...

ulupo commented 4 years ago

Let's leave this as a "low priority" for now. Time allowing after the other PRs, I'll propose a temporary patch.