giotto-ai / giotto-tda

A high-performance topological machine learning toolbox in Python
https://giotto-ai.github.io/gtda-docs
Other
845 stars 173 forks source link

Add DensityFiltration #473

Closed gtauzin closed 3 years ago

gtauzin commented 4 years ago

Adds DensityFiltration for binary images.

Types of changes

Checklist

gtauzin commented 4 years ago

A general question I have is whether it is clear to the user what passing non-default metric_params means exactly. What is the "cosine distance" between pixels in an image? I haven't yet finished reading the code in detail but just from documentation I find it a bit obscure.

It's a fair point. I guess, most of the time, you won't need to pass non-default metric_params. As far as calculating cosine distances of pixels, I guess if your image have been obtained from a complex preprocessing/ measurement, pixel positions may have strong meaning and you might want to do that. But really, I have never used it. Same goes for point clouds actually (with the exception of our work xD).

ulupo commented 4 years ago

I guess, most of the time, you won't need to pass non-default metric_params

Again today, I didn't write what I meant. I meant "non-default metric", which is more important I guess. I'm playing the part of a user affected by intellectual misery who might not understand even what it means to use a metric in this context. Again, not having looked at the code (somewhat on purpose for my user simulation), I guess that the activated pixels are converted into an integer grid in 2D just as in ImageToPointCloud, and then the metric is just a metric in R^2 applied to these pixels seen as points with integer coordinates?

gtauzin commented 4 years ago

@ulupo I think it would be better to wait right before merging the PR to pull from master so that we have to handle the conflicts only once.

gtauzin commented 4 years ago

I guess, most of the time, you won't need to pass non-default metric_params

Again today, I didn't write what I meant. I meant "non-default metric", which is more important I guess. I'm playing the part of a user affected by intellectual misery who might not understand even what it means to use a metric in this context. Again, not having looked at the code (somewhat on purpose for my user simulation), I guess that the activated pixels are converted into an integer grid in 2D just as in ImageToPointCloud, and then the metric is just a metric in R^2 applied to these pixels seen as points with integer coordinates?

I have updated the transformer description. Is it more understandable now?

gtauzin commented 4 years ago

@ulupo The tests in tests/test_common.py are now failing. Any idea why?

ulupo commented 4 years ago

@gtauzin yes. See https://github.com/giotto-ai/giotto-tda/pull/473#discussion_r492203832

gtauzin commented 3 years ago

@ulupo Apart from your approval on the updated docstring, it's ready to merge for me :)

gtauzin commented 3 years ago

I'm a bit worried about the pybind11 submodule update which is included in this PR. Was it intentional? Could you explain?

No idea, but I reverted it.

I also added inline comments in the code.