henselman-petrusek / Eirene.jl

Julia library for homological persistence
Other
117 stars 21 forks source link

wasserstein_distance is not symmetric #43

Closed dbhaskar92 closed 2 years ago

dbhaskar92 commented 3 years ago

Output of optimized code (commit a214726 onwards) depends on the order of arguments. To reproduce:

1) Generate data:

    julia> using Random
    julia> using Eirene
    julia> Random.seed!(123);
    julia> rescale(coords) = (coords .- maximum(coords))./(maximum(coords)-minimum(coords))
    julia> pt_cloud_1 = rescale(noisytorus(m=30, n=10));
    julia> pt_cloud_1[:,1]
    3-element Vector{Float64}:
     -0.027817932268719984
     -0.39172231301472105
     -0.454187098172808
    julia> pt_cloud_2 = rescale(noisycircle3());
    julia> pt_cloud_2[:,1]
    3-element Vector{Float64}:
     -0.0005529205231942241
     -0.5368438485462651
     -0.5630209048408695

2) Compute barcodes:

    julia> pers_diag_1 = eirene(pt_cloud_1, model="pc", maxdim=1);
    julia> pers_diag_2 = eirene(pt_cloud_2, model="pc", maxdim=1);
    julia> barcode_1_H1 = barcode(pers_diag_1, dim=1);
    julia> barcode_2_H1 = barcode(pers_diag_2, dim=1);

3) Print Wasserstein distances:

Testing with other datasets, I found that W(X,Y) is close to W(Y,X) in commit b38f8af or earlier , but significantly different in later versions.

Can you please help me understand the discrepancy? I'm running Julia 1.6.2, happy to assist in testing/debugging the code :)

henselman-petrusek commented 3 years ago

Thanks for the report! I'll defer to the Wasserstein authority on this.

yossibokorbleile commented 3 years ago

Jumping on to @Eetion thanks, thanks for the report! I have had a look, and think I have managed to localise the issue (without knowing exactly what caused it yet), and have a beta fix. @dbhaskar92 if you are happy to test it, I can provide you with the (stripped back) fix.

yossibokorbleile commented 3 years ago

If you look at my fork and commit 434e891 it should be resolved. Edited 15/09/2021: changed the commit reference above as there was an extra 's' at the bottom of the file in the previous commit.

yossibokorbleile commented 2 years ago

hey @dbhaskar92 just hoping to give this a bump and see if you have continued to encounter issues.

dbhaskar92 commented 2 years ago

@yossibokor No, I did not encounter any further issues in commit 434e891, thanks so much!

yossibokorbleile commented 2 years ago

Great! Then we can close the issue @Eetion