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

Fix issue 403 (color_variable is scaled in plot_static_mapper by default) #405

Closed MikeG112 closed 4 years ago

MikeG112 commented 4 years ago

Reference issues/PRs Fixes #403 via one possible resolution.

Types of changes

Description Issue #403 notes that when a color_variable is selected for use in plot_static_mapper, the existing behavior is to scale the node color mapping so that it takes values in [0,1]. While this is a reasonable default behavior in principle, it is somewhat at odds with the fact that the default behavior for displaying other data columns in the drop down list is to display the node color mappings without normalizing.

The proposed fix here is to add a paramater normalize_node_colors to plot_static_mapper and the various functions called from plot_static_mapper that are involved in the node coloring process (e.g. in _get_node_colors), and then to default this parameter to False at every stage. I did not yet add any new tests for this change in behavior, because the nature of tests will depend on the nature of the implemented fix, so I thought I'd get some feedback on this proposal first.

Any other comments?

Checklist

ulupo commented 4 years ago

@wreise, @lewtun and maybe @WeilerP remember this better but I seem to recall that the normalization was actually needed to obtain reasonable colours, because of some quirks in the colorscale mapping we use. If this is the case then I'm not sure it is reasonable to have normalize_node_colors=False when the function is actually called.

MikeG112 commented 4 years ago

@ulupo That makes sense, I will try taking a look for erratic color choice behavior when the normalization is not set. If that does indeed occur, it may make sense to update the default to True, but in that case my feeling is that it would also be appropriate to change the default behavior to normalize all of the variables in the drop down list (rather than only the column selected).

In any case, it may be easier to just deal with this all in your larger refactor #406. If you are interested in merging this branch at some point, please let me know and I will be happy to look into your suggestions, otherwise I will assume #406 will fix issue #403.

ulupo commented 4 years ago

@MikeG112, it seems indeed that it would be too difficult to implement both the changes as made here and as made (alongside a host of new features and improvements) in #406. I'll close this PR but please let me know if you think this is a mistake. Your feedback to #406 would also be extremely welcome (I need to update the description there so the changes are more clearly explained).

MikeG112 commented 4 years ago

Thanks @ulupo, that sounds good to me. I will take a look at #406 some time soon and let you know if I have any thoughts!