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

More Mapper tests, small change to `max_fraction` in FirstSimpleGap and FirstHistogramGap #412

Closed wreise closed 4 years ago

wreise commented 4 years ago

Reference issues/PRs None

Types of changes

Description

Checklist

ulupo commented 4 years ago

@wreise I was able to put together a version which "works". I have not got to the bottom of the issue but it seems that we have bad conflicts/side effects between tests. It's as if the fig or pipe objects were changed simultaneously by different tests, causing the failures (but again, not sure). Placing the tests for plot_interactive_mapper_graph in a new file seems to have "solved" this. (?!)

ulupo commented 4 years ago

@wreise @lewtun the mystery is solved! The point was that ipywidgets somehow links together all widgets created in the same session, so that even after the Python object fig has seemingly been overwritten, fig.widgets still contains data from previously created widgets. This caused a problem when using @wreise's _get_widget_from_trait because this function could fetch the widget belonging to a previously created figure, instead of the correct one. This meant that fig did not actually observe a change, despite us thinking we caused one!

My fix involves turning _get_widget_from_trait into _get_widgets_from_trait which now returns a list of all possible matches. In the test code, all possible matching widgets are changed. It's not elegant (because ideally we would only change what's relevant), but it works!

@wreise I am hoping I can leave this PR for you to finish off, if you think you would be able to. As is, it already boosts code coverage substantially, but there are a few simple things which can be added (e.g. use of pandas dataframes, and maybe passing color_by_columns_dropdown=True). The coverage report (pytest gtda/mapper --cov --cov-report xml) is a good guide.

wreise commented 4 years ago

Thank you for your thorough investigation, @ulupo ! I should be able to finish this.

ulupo commented 4 years ago

@wreise thanks! And thanks for opening this a long time ago, even if we didn't know how to do it then. It served as a good reminder.