Replace earthmover's distance implementation #280
Conversation
|
|
|
Is including the license at the bottom of |
|
Seems confusing/troubling that the build passed before I had added |
|
And now it's failing with |
|
The tests passed in the previous case because NBD wasn't imported in the case of an |
| @@ -16,5 +16,3 @@ python: | |||
| - requirements: requirements.txt | |||
| - method: pip | |||
| path: . | |||
| extra_requirements: | |||
There was a problem hiding this comment.
I'm not following why this was removed?
There was a problem hiding this comment.
My read is that the doc 'extra_requirements in .readthedocs.yml is the same doc extra_requirements in setup.py, which was just POT, and which is also removed in this PR. Is that wrong?
There was a problem hiding this comment.
Ohhh wait I think you're right! We will have to see if this build the docs correctly, but I bet you're right.
Which makes me feel Bad for not having a test branch.
netrd/distance/nbd.py
Outdated
| dist = emd2(mass(vals1.shape[0]), mass(vals2.shape[0]), vals_dist) | ||
|
|
||
| vals1 = [tuple(vals1[i]) for i in range(len(vals1))] | ||
| vals2 = [tuple(vals2[i]) for i in range(len(vals2))] |
netrd/distance/nbd.py
Outdated
| # This file borrows code (euclidean_distance and earthmover_distance) with the | ||
| # following license: | ||
| # | ||
| # MIT License |
There was a problem hiding this comment.
Maybe add the License inside earthmover_distance instead of at EOF?
This resolves #232 and helps with #267 by replacing the POT-based earthmover's distance implementation with Jeremy Kun's newly MIT-licensed implementation based on
ortools. His implementation is not on pypi so the relevant functions are reproduced (with the license) innbd.py. This removes the Cython and POT workarounds we had in.travis.ymlandrequirements.txtto handle its installation. This will hopefully make for a smoother experience with test builds.