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

Make bindings public #395

Closed gtauzin closed 4 years ago

gtauzin commented 4 years ago

Signed-off-by: Guillaume Tauzin guillaumetauzin.ut@gmail.com

Types of changes

Description Allow the user to import and use our bindings in his/her own project. For now, the alternative is simply to copy and paste all the bindings files.

Checklist

ulupo commented 4 years ago

@gtauzin thanks! This is certainly a good thing. But just to be sure I understood:

For now, the alternative is simply to copy and paste all the bindings files.

I don't understand this. I'm able to e.g. run

from gtda.externals.modules.gtda_bottleneck import bottleneck_distance

from a jupyter notebook not in the project's root, and it does not fail.

gtauzin commented 4 years ago

@gtauzin thanks! This is certainly a good thing. But just to be sure I understood:

For now, the alternative is simply to copy and paste all the bindings files.

I don't understand this. I'm able to e.g. run

from gtda.externals.modules.gtda_bottleneck import bottleneck_distance

from a jupyter notebook not in the project's root, and it does not fail.

Well spotted, it does work! makes sense as it is actually what we do inside the library itself for modules :).

I do think it is still a goo idea to have them all listed up and available directly at the externals directory level in init.py as it simplifies imports. It's also cleaner as it follows all other submodules standard when it comes to what is public and what is not. All hidden functions can also be imported using their relative path. I.e., this will work:

from gtda.diagrams._utils import _subdiagrams

_subdiagrams is supposed to be private (and it is also undocumented as a result). On the other hands, bindings are all documented and could be made fully available to the user. That would be also a way to promote @MonkeyBreaker 's pybind11 faster bindings counterparts of what is already existing. IMO, the documentation would need to be adapted before it is published. I can write an issue for that.

But what you just pointed out already solves my problem, so thanks!