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

WIP: Add earth mover's distance #4

Closed
wants to merge 1 commit into from
Closed

WIP: Add earth mover's distance #4

wants to merge 1 commit into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 8, 2018

This depends on the unregistered https://github.com/mirkobunse/EarthMoversDistance.jl. Issues include dependence on a C library. Thoughts?

@timholy
Copy link
Member Author

timholy commented Oct 10, 2018

See some discussion at robertfeldt/EarthMoversDistance.jl#1

@juliohm
Copy link
Member

juliohm commented Oct 10, 2018

Thank you @timholy I fully agree with the discussion you've linked. It would be nice to have a Julia implementation of EMD algorithms. I am also busy at the moment to try understand it in depth.

Do you think it is a good idea to have it here as a dependency though? My opinion is that we could make use of the unregistered package, or maybe register it for standalone usage until someone finds time to implement it in Julia.

@timholy
Copy link
Member Author

timholy commented Oct 10, 2018

See JuliaLang/Pkg.jl#810

@juliohm
Copy link
Member

juliohm commented Oct 10, 2018

Interesting issue, thanks for sharing. My comment was more about maybe leaving EarthMoversDistance.jl the way it is as a separate package with a C dependency until someone finds the time to implement a pure Julia version?

Another issue is that the Earth movers distance has an original definition with probability distributions. Maybe one could define the distance in a package where non-parametric (histogram) distribution types are available. Perhaps Distances.jl or Distributions.jl would be more appropriate places for this algorithm? Hard to say. In any case, we could import a package like Distances.jl or Distributions.jl into ImageDistances.jl and wrap the algorithm to work with images.

@timholy
Copy link
Member Author

timholy commented Oct 10, 2018

There really isn't anything terribly image-specific here, if the main algorithm lives elsewhere that's pretty much it. Shall I close?

@juliohm
Copy link
Member

juliohm commented Oct 10, 2018

The C dependency is a major downside, let's close it for now. We can get back to it in the future with more time to make it Julian. Thanks for raising the issue and for the reference implementation.

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