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

Hausdorff distance to return the pair of points that defines it #5178

Closed
ogencoglu opened this issue Jan 8, 2021 · 20 comments
Closed

Hausdorff distance to return the pair of points that defines it #5178

ogencoglu opened this issue Jan 8, 2021 · 20 comments

Comments

@ogencoglu
Copy link

Description

Thanks for the new metric of hausdorff_distance: https://scikit-image.org/docs/dev/api/skimage.metrics.html#skimage.metrics.hausdorff_distance . In many cases it is quite relevant to also get the pair of pixel coordinates that defined the Hausdorff distance, i.e., one coordinate from image0 and another from image1 that gave the maximum distance.

@rfezzani
Copy link
Member

rfezzani commented Jan 8, 2021

Thank you @ogencoglu for this suggestion 😉

@ogencoglu
Copy link
Author

For example scipy hausdorff returns the indices respectively: https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.directed_hausdorff.html

@clementkng
Copy link
Contributor

As one of the original authors of the hausdorff distance metric, I can try implementing this.

@jni
Copy link
Member

jni commented Jan 19, 2021

Hi @clementkng and @ogencoglu!

I think this would be a useful addition. The biggest issue with implementing it is that it becomes an API change. In general, we are also trying to move away from APIs like return_points=True that change the number of return values, because they make typing/predicting the behaviour of a program more difficult.

@scikit-image/core any ideas about the best way forward is here? tbh I'm inclined to make an exception and allow return_points as an extra kwarg.

@ogencoglu
Copy link
Author

Here are the 2 options:

  1. No extra return_points argument. It just returns distance and the points, just like scipy. I understand that this might effect other processes that uses this function and those need to be adjusted as well.
  2. Whether return_points is True or False it will always return the same number of return values. Maybe a tuple, e.g. (dist, points). If return_points is False, points will be None.

@rfezzani
Copy link
Member

What about creating an extra function that returns the coordinates of the points? This way no need to modify hausdorff_distance...

@jni
Copy link
Member

jni commented Jan 19, 2021

@ogencoglu the above two options will cause user code to break for users that use 0.18.x. We don't do that in a single release except in case of bug fixes.

@rfezzani yes that's the other alternative, but it seems ugly/clunky. I think we did settle for this as the approach for 1.0, with the new function called hausdorff_distance_extras(). Possibly that's my preferred option.

@mkcor
Copy link
Member

mkcor commented Jan 19, 2021

Having an additional function hausdorff_distance_extras() which would wrap hausdorff_distance() seems ugly, indeed. I would lean in favour of making an exception... naming the extra kwarg return_coords.

@rfezzani
Copy link
Member

My suggestion was to add hausdorff_peaks (may be not the best function name :/) function that returns only the points coordinates.

@stefanv
Copy link
Member

stefanv commented Jan 19, 2021

We already know that returning varying number / types of arguments is a problem for future typing, so I'd avoid creating additional headaches for ourselves knowingly. We need a SKIP for this, but for this PR that will come too late; as such, we have a few (non-headache inducing) options:

  1. Always return coordinates, setting it to None if not calculated.
  2. Always return first, second, extras, with extras having a key that corresponds to coordinates (I think this most closely aligns with the pattern SciPy uses).

I'm more fond of option (2) than of two separate functions, but separate functions at least avoid the typing problem.

@mkcor
Copy link
Member

mkcor commented Jan 20, 2021

@rfezzani my bad, I misunderstood your suggestion indeed. A separate, 'non_overlapping' function would certainly be more acceptable, even if not ideal.

@stefanv ok, so we would modify the existing hausdorff_distance() function (breaking existing code?) to always return distance, extras and then access the coordinates with dictionary keys, e.g., extras['image0_coords']? Or even with object attributes (extras.image0_coords) as in the SciPy docs you linked to?

@stefanv
Copy link
Member

stefanv commented Jan 20, 2021

@mkcor Right, so it would take a deprecation cycle with an extras (or similar) keyword, I imagine.

@clementkng
Copy link
Contributor

So if I'm reading the above conversation correctly, it seems that @mkcor and @stefanv are partially leaning in favor of

Always return first, second, extras, with extras having a key that corresponds to coordinates (I think this most closely aligns with the pattern SciPy uses)

while @rfezzani and @jni are partially leaning in favor of

add hausdorff_peaks (may be not the best function name :/) function that returns only the points coordinates

I have a slight preference for the hausdorff_peaks, but only because the implementation is more clear to me. Anyone else available to tiebreak?

@stefanv
Copy link
Member

stefanv commented Jan 23, 2021

I am in favor of any option that doesn't paint us into a corner, so housedorff peaks works too.

@jni
Copy link
Member

jni commented Jan 24, 2021

hausdorff_points?

@rfezzani
Copy link
Member

I see in the description of the output of scipy.spatial.distance.directed_hausdorff the use of "hausdorff pair".
Isn't then hausdorff_pair or hausdorff_pair_points a good function name?

@clementkng
Copy link
Contributor

I think hausdorff_pair is a good function name since it's consistent with scipy.

@clementkng
Copy link
Contributor

@ogencoglu can we close this issue now that #5207 is merged?

@rfezzani
Copy link
Member

rfezzani commented Mar 5, 2021

Thank you @clementkng for the reminder 😉 and thank you @ogencoglu for your suggestion!

@rfezzani rfezzani closed this as completed Mar 5, 2021
@ogencoglu
Copy link
Author

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants