[MRG] Sliced wasserstein #203
Conversation
This reverts commit 9f51c14
|
It doesn't build because of the string formatting: f"stuff" which is only supported from python 3.6. You guys sure you want to support python 3.5? |
|
yes we keep it, we can have this discussion for the next major release but for now we still support 3.5. Note that you have a lot of things in this PR that should not be here (and in other PR):
|
|
Sorry did not see the revert up there about log ;) |
|
I reverted a bunch of commits in this branch, you should compare the files,
not look at the history (sinkhorn in log is not there anymore).
Correct for the docstring. I was sloppy with my branches, so I'm just
waiting for the doc one to be merged so the branch will be up to date.
Le lun. 20 juil. 2020 à 14:56, Rémi Flamary <notifications@github.com> a
écrit :
… yes we keep it, we can have this discussion for the next major release but
for now we still support 3.5.
Note that you have a lot of things in this PR that should not be here (and
in other PR):
- Change for sinkhorn in log (i agree for a new solver but it should
be a new one, you do not change the current default)
- Correction for the doc of barycenter (already in another PR)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#203 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYGFZ5X2FXQOFVU3C4L5JDR4QWFDANCNFSM4PCHDJXA>
.
|
|
done and updated your PR (merged master) |
|
Cool I corrected the string formatting for py3.5 support.
Also honestly, all this SWD should probably be done directly in cython as
the calculation can be vectorized, but I guess that can be left for a
hypothetical future...
Le lun. 20 juil. 2020 à 15:03, Rémi Flamary <notifications@github.com> a
écrit :
… done and updated you PR
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#203 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYGFZ3OEVACFKPF5Q3DAWLR4QXC3ANCNFSM4PCHDJXA>
.
|
|
Meant parallelized, not vectorized.
Le lun. 20 juil. 2020 à 15:05, Adrien Corenflos <adrien.corenflos@gmail.com>
a écrit :
… Cool I corrected the string formatting for py3.5 support.
Also honestly, all this SWD should probably be done directly in cython as
the calculation can be vectorized, but I guess that can be left for a
hypothetical future...
Le lun. 20 juil. 2020 à 15:03, Rémi Flamary ***@***.***> a
écrit :
> done and updated you PR
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#203 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEYGFZ3OEVACFKPF5Q3DAWLR4QXC3ANCNFSM4PCHDJXA>
> .
>
|
|
Cool, looks like it's building just fine |
There was a problem hiding this comment.
Thank you very much @AdrienCorenflos for this PR.
I did a code review and found several comments below that should be addressed before merging.
Also please do the following:
- Add the new module to the doc
- Provide a simple example in 2D for a large number of samples of the computation of the sliced distance (maybe compute a variance as a function of the number of projection). The example should be in the folder below should follow the other examples format and will automatically be added in the documentation:
https://github.com/PythonOT/POT/tree/master/examples/others - Add your name to the contributors in the Readme file and the sliced wasserstein to the list of features (with a link to the example).
Thank you again for the PR there is a little bit of work before merging but it will be a nice contribution to POT.
|
Thanks for the thorough review. I'll amend all this early next week.
Adrien
…On Fri, 31 Jul 2020, 17:01 Rémi Flamary, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ot/sliced.py
<#203 (comment)>:
> +#
+# License: MIT License
+
+
+import numpy as np
+
+
+def _random_projections(n_projections, dimension, random_state):
+ """Samples n_projections times dimension normal distributions"""
+ projections = random_state.normal(0., 1., [n_projections, dimension])
+ norm = np.linalg.norm(projections, ord=2, axis=1, keepdims=True)
+ projections = projections / norm
+ return projections
+
+
+def sliced(X_s, X_t, a=None, b=None, n_projections=50, seed=None):
Also you should name the function sliced_wasserstein_distance
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#203 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYGFZ675GRJHRM5DFVPEKTR6LFCTANCNFSM4PCHDJXA>
.
|
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
+ Coverage 92.28% 92.38% +0.09%
==========================================
Files 15 16 +1
Lines 3006 3045 +39
==========================================
+ Hits 2774 2813 +39
Misses 232 232 |
|
Feels like it should be good @rflamary |
|
@rflamary I think I included all your comments, do you see anything left? |
|
Sorry this is an hectic time on my end, I see some small comments but don't have time for a proper code review yet. Will do it asap. Only minor things remain thank you for taking into account all my comments. |
|
Btw on the back of this I can (very) easily do the (heuristical max slice), is that something that would be of interest? |
|
Anything else @rflamary ? |
|
Hello, Thank you @AdrienCorenflos . I'm OK with the PR and plan on merging it. But the build_linux_minimal fails and I want to investigate this problem before merging. It does not come from you PR but I don't wan't to merge if the build fails. |
|
Maybe you'd want to rerun the checks? |
|
The problem is that if I rerun the checks they might pass but that something we have to understand and Im' pretty busy right now ;). I promise I will merge your PR before the next release but I want to solve the mystery before pilling merges. |
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
+ Coverage 92.28% 92.38% +0.09%
==========================================
Files 15 16 +1
Lines 3006 3045 +39
==========================================
+ Hits 2774 2813 +39
Misses 232 232 |
Types of changes
Motivation and context / Related issue
Implement SWD: #202
How has this been tested (if it applies)
Added specific tests (positive definiteness + matching the EMD in the 1D case)
Checklist
Not sure why yet but the stuff doesn't build.
I'm publishing this as a draft as I have some other changes in my branch that are pending for another merge (cf this: #200)