Skip to content

Conversation

hichamjanati
Copy link
Contributor

@hichamjanati hichamjanati commented Jun 12, 2019

new unbalanced module for UOT with KL relaxation with the funcs:

  • sinkhorn_unbalanced: generalized Sinkhorn to compute W

  • barycenter_unbalanced: unbalanced Wasserstein barycenter

  • Tests of convergence for both algorithms

  • Examples plot_UOT_1D and plot_UOT_barycenter_1D with unbalanced gaussian distributions.

@hichamjanati
Copy link
Contributor Author

If you agree with this first draft, I'll work on followup PRs for log-domain, barycenters, convolutional UOT for images, and GPU compatibility.

@ncourty
Copy link
Collaborator

ncourty commented Jun 12, 2019

Great ! Thanks Hicham !

Copy link

@massich massich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change address those changes, modify the title to only adding one type and do the rest in subsequent PRs. that would simplify the review process.

@hichamjanati hichamjanati marked this pull request as ready for review June 12, 2019 15:08
@hichamjanati hichamjanati changed the title Cover Unbalanced OT with KL relaxation Add Unbalanced KL Wasserstein distance + barycenter Jun 12, 2019
@rflamary
Copy link
Collaborator

Hello @hichamjanati,

Thank you very much indeed. I will have a look at your code and do a review myself but it looks great.

Copy link
Collaborator

@rflamary rflamary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @hichamjanati

Thank you this is a nice PR, here are some comments.

Rémi

@rflamary rflamary changed the title Add Unbalanced KL Wasserstein distance + barycenter [WIP] Add Unbalanced KL Wasserstein distance + barycenter Jun 18, 2019
@hichamjanati
Copy link
Contributor Author

  • I added a test for sinkhron_unbalanced2 and for the method argument as @massich suggested.
  • Also one more test for a multidimensional distribution b.
  • Adapted the docstring examples
    Unless you have any other requests, it's ready for merge :)

@rflamary
Copy link
Collaborator

Hello, this is great.

Could you please build the doc with the makefile in the doc folder with make html and check that all your new fonction build correctly please (readthedoc dont provide build for PR).

@rflamary
Copy link
Collaborator

Also don't forget to update the Readme with:

  • your name in the contributors
  • the new reference from frogner
  • the unbalanced OT in the feature list at the top

@hichamjanati
Copy link
Contributor Author

@rflamary Thanks for the feedback
Running make html in the doc folder did not build my examples and it turns out something is really wrong with the doc ... it's also the case for many previous PRs (Fused gromov #86, #80, #73) and potentially others. The changes in those PRs do not appear on https://pot.readthedocs.io
I tracked down the last working example and it's convolutional wasserstein (#64) but I could not find any wrong changes on the doc config since then ...
any ideas ?

@rflamary
Copy link
Collaborator

Hello,

The doc builds on my end and on readthedoc so I don't think there is really something wrong with it. The documentation including the latest PR is available in the 'latest' version of the doc:
https://pot.readthedocs.io/en/latest/index.html
you were looking at the stable version that corresponds the last release (on pip and conda).

Note that in order to build the doc you need sphinx-gallery . If the doc do not build, you should open an issue and describe what fails. maybe it requires some more dependencies. I am working on a big doc PR in #88 before the next release where I will do some cleanup so i can address the problem.

@hichamjanati
Copy link
Contributor Author

Sorry I put the wrong link but even in the latest version, the Fused-Gromov examples plot_fgw.py and plot_barycenter_fgw.py are not available. Their .rst versions are not in docs/source/auto_examples. I'll narrow down the issue to see what's wrong and create an issue.

@rflamary
Copy link
Collaborator

OK i see,

Don't worry, there is not problem. I add the examples semi-manually to the documentation before release because the sphinx-gallery cannot run on readthedoc (you cannot compile stuff). But you have a point, it should be written somewhere, I will add a Readme in the doc folder.

What I asked wrt the documentation is that you check that the doc build and that your functions have a correct format (rst is so tricky). I will take care of adding the examples and the notebooks.

Copy link
Collaborator

@rflamary rflamary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @hichamjanati ,

we are near merging now.

I did a checkout of your code and the doc builds but there are still a few typos and details missing from the doscstrings (see comments).

Also if you want you new module to appear in the documentation, you need to add it to https://github.com/rflamary/POT/blob/master/docs/source/all.rst. All the functions will be automatically documented. Don't worry about the examples in the documentation, i will take care of it in a next PR.

@hichamjanati
Copy link
Contributor Author

Ah okay, I see now. I have made the requested changes on the docstrings and init and added the module to source/all.rst.

@rflamary rflamary changed the title [WIP] Add Unbalanced KL Wasserstein distance + barycenter [MRG] Add Unbalanced KL Wasserstein distance + barycenter Jun 25, 2019
@rflamary
Copy link
Collaborator

Ok @hichamjanati, I will do the merge now.

Thank you for this very nice contribution, please I think the stabilized and gpu versions would be very nice follow ups to this PR.

@rflamary rflamary merged commit 2364d56 into PythonOT:master Jun 25, 2019
@rflamary
Copy link
Collaborator

Hello Hicham,

I'm still working on it but you can see the upcoming documentation here:
https://pot.readthedocs.io/en/doc_modules/

you can see the FGW and your examples here and they work like a charm

@hichamjanati
Copy link
Contributor Author

Hello Rémi,
Ah great, beautiful indeed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants