-
Notifications
You must be signed in to change notification settings - Fork 521
Wasserstein Circle distance doesn't seem correct? #738
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
Comments
You are right, I just proposed in #PR736 to change the default behaviour of |
Okay, good to know I'm not going crazy 😅 Have you considered using hypothesis to property-test this library? It works particularly well for mathematical code with lots of clean invariants / symmetries. This property-test immediately finds a minimal counterexample: import hypothesis.strategies as st
from hypothesis import given
import numpy as np
import ot
@given(
arr1=st.lists(
st.floats(min_value=0, max_value=1),
min_size=1, max_size=10,
),
arr2=st.lists(
st.floats(min_value=0, max_value=1),
min_size=1, max_size=10,
),
delta=st.floats(min_value=-1, max_value=1)
)
def test_wasserstein_circle_is_rotationally_symmetric(arr1, arr2, delta):
arr1 = np.array(arr1)
arr2 = np.array(arr2)
[d1] = ot.wasserstein_circle(arr1, arr2)
[d2] = ot.wasserstein_circle((arr1 + delta) % 1, (arr2 + delta) % 1)
assert np.isclose(d1, d2, atol=1e-3)
The |
Actually not quite, this gives a warning: E RuntimeWarning: invalid value encountered in divide
E Falsifying example: test_binary_search_circle_is_rotationally_symmetric(
E arr1=[0.5],
E arr2=[0.25],
E delta=0.5,
E ) Standalone example: import numpy as np
import ot
import warnings
warnings.filterwarnings("error")
arr1 = np.array([0.5])
arr2 = np.array([0.25])
delta = 0.5
[d1] = ot.binary_search_circle(arr1, arr2)
[d2] = ot.binary_search_circle((arr1 + delta) % 1, (arr2 + delta) % 1)
assert np.isclose(d1, d2, atol=1e-3)
Not sure if this is a real bug but thought it was worth recording. |
Sorry, it's my bad ^^. I shouldn't have use this implementation for the default behaviour... I didn't know the hypothesis library. It seems very useful! I am reassured that Concerning the |
Uh oh!
There was an error while loading. Please reload this page.
Describe the bug
Forgive me if I've misunderstood what the
wasserstein_circle
function is supposed to do. But I would have thought that if we haveAnd then we add the same amount to both arrays (i.e. rotating both samples the same angle around the circle), then we should get the same answer.
But the first example I tried fails.
To Reproduce
Expected behavior
wasserstein_circle
should be rotationally symmetric, i.e. it should obey the propertyFor all real
delta
(up to floating point inaccuracies), because this amounts to just turning your head to the side.Or am I just misunderstanding how the input is supposed to be represented here?
Environment (please complete the following information):
pip
,conda
): NixOutput of the following code snippet:
Additional context
The text was updated successfully, but these errors were encountered: