Add curves.Derivative - 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

Add curves.Derivative #492

Closed gtauzin closed 4 years ago

gtauzin commented 4 years ago

Reference issues/PRs

Types of changes

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

Description Add curves.Derivative. See #480.

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.
[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.

wreise commented 4 years ago

Since the tests were failing, i took the initiative to fix them. In the process, i made Derivative inherit from PlotterMixin: it is a bit delciate though, because we do not have the samplings. For the sake of computations, we assume it's uniform, but i think we lack scale/position information for plotting (the x-axis). @gtauzin, @ulupo : what do you think about the current proposal and do you have suggestions on how to bypass these problems ?

gtauzin commented 4 years ago

@wreise Fine with me!

gtauzin commented 4 years ago

@ulupo I think one the test failed because of the CI itself. They should be ran again.

Also this PR does not depend on the curves array shape as the derivative is taken on axis=-1, so it can be merged independently from #480.

ulupo commented 4 years ago

@gtauzin just pointing out that, to be consistent with StandardFeatures, I have added shape checking which only allows for 3D input (I know the transformer would not fail, it's just for consistency/guiding the user). And I have added a n_channels_ attribute again to be consistent with StandardFeatures. Are you happy with these changes and shall I merge?

gtauzin commented 4 years ago

@ulupo Nothing to say, that's perfect !

Thank you!