Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make bindings public #395

Merged
merged 2 commits into from
May 12, 2020
Merged

Make bindings public #395

merged 2 commits into from
May 12, 2020

Conversation

gtauzin
Copy link
Collaborator

@gtauzin gtauzin commented Apr 9, 2020

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

Types of changes

  • 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
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

  • I have read the guidelines for contributing.
  • 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.
  • I have added tests to cover my changes.
  • All new and existing tests passed. I used pytest to check this on Python tests.

Signed-off-by: Guillaume Tauzin <guillaumetauzin.ut@gmail.com>
@gtauzin gtauzin requested a review from ulupo April 9, 2020 23:13
@ulupo
Copy link
Contributor

ulupo commented Apr 10, 2020

@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
Copy link
Collaborator Author

gtauzin commented Apr 12, 2020

@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!

@ulupo ulupo merged commit cb3c81b into giotto-ai:master May 12, 2020
ulupo added a commit that referenced this pull request Jun 2, 2020
* Fix issue with docstring example and document reshaping of 1D outputs (#396)

* Toc (#394)

* Update P landscapes

Signed-off-by: ammedmar <anibal@medina-mardones.com>

* Add distances, inner products and kernels glossary entry

Signed-off-by: ammedmar <anibal@medina-mardones.com>

* Remake vectorization changes

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Change [] for \lbrack \rbrack

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after W's comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after W's comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update afte W's second comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's second comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's third comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's 4th comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Remove concept k-skeleton

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Add table of content

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Add Lp & lp. Update landscape

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update TOC indentation

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Add heat vectorizations entry

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update indentation of TOC

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Fix extra spacing in bibliography

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update bibliography hack for caps

Signed-off-by: ammedmar <ammedmar@gmail.com>

* After Umbe's Comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after issue #398

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

* Update after Umbe's 2nd comments

Signed-off-by: ammedmar <ammedmar@gmail.com>

Co-authored-by: ammedmar <anibal@medina-mardones.com>

* Speedup windows pipeline (#402)

* Improve boost location for azure pipeline on windows

The boost version installed in the pipeline is now used

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Co-authored-by: Umberto Lupo <46537483+ulupo@users.noreply.github.com>

* Make bindings public (#395)

* Make bindings public

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

* Fix pipeline on Mac (#407)

* Refresh ccache

* Enforce CXX standard to 14 on each module

* Change variable name to comply with E741

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Co-authored-by: Umberto Lupo <46537483+ulupo@users.noreply.github.com>

* Mapper visualisation refactor: fix bugs, add summary statistics in hovertext, improve opacity, remove matplotlib dependency, add node_scale kwarg, add clone_pipeline kwarg to interactive plots,restructure/rename plotly_kwargs, improve code (#406)

* Refactor of `mapper/visualisation.py` and `mapper/utils/visualisation.py`
- Removal of color scaling
- More modularity
- Variable and function name changes
- Remove matplotlib functions used for hoverlabel background color
- Remove cmin and cmax
- Change return signature of some functions

* Add test-output.xml to .gitignore

* Replace "text" key in plot_options["node_trace"] with "hovertext"

* Improve width and opacity of nodes and edges

* Display summary statistics in hovertext with significant figure rounding via new n_sig_figs kwarg

* Rename plotly_kwargs to plotly_params

* Remove matplotlib installation requirement

* Fix mapper notebook

* Add small comment on use of matplotlib in voids_on_the_plane

* Place matplotlib in examples requirement in setup.py

* Improve docstring of make_mapper_pipeline

* Improve examples for make_mapper_pipeline and create one for plot_static_mapper_graph

* Hide set_node_sizeref, add node_scale kwarg, add clone_pipeline kwarg to plot_interactive_mapper_graph

- Update docstrings
- Update quickstart notebook

* Add helper function for colorscale-based interpolations, improve use of plotly_params

Static and interactive plots display the expect hoverlabel colors in 3D, or white if things go wrong. Thanks to @lewtun for pointing out that this was needed.

* Improve docstrings

* Hide visualization module in mapper/utils

* Add pip install matplotlib to notebook tests in manylinux job (#410)

* Fix mapper docstring issues following #406 (#411)

* Fix issues with mapper docs following #406

* Create giotto-tda version 0.2.2 (#413)

* Turn CODE_OWNERS and CODE_AUTHORS into an rst file

* Bump version number to 0.2.2

* Add release notes for v0.2.2

Co-authored-by: Anibal M. Medina-Mardones <ammedmar@gmail.com>
Co-authored-by: ammedmar <anibal@medina-mardones.com>
Co-authored-by: REDS institute <reds-heig@users.noreply.github.com>
Co-authored-by: Guillaume Tauzin <guillaumetauzin.ut@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants