Skip to content
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

Implementation of Low Rank Gromov-Wasserstein #614

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

laudavid
Copy link
Contributor

@laudavid laudavid commented Mar 8, 2024

Types of changes

This PR is for the implementation of the Low Rank Gromov-Wasserstein solver.

Paper reference:
Scetbon, M., Peyré, G. & Cuturi, M. (2022).
"Linear-Time GromovWasserstein Distances using Low Rank Couplings and Costs".
In International Conference on Machine Learning (ICML), 2022.

Changes made:

  • Created the lowrank_gromov_wasserstein function in ot/lowrank.py
  • Created the _flat_product_operator function in ot/lowrank.py since it is needed for the low rank GW solver
  • Added tests for each new functions in test\lowrank.py
  • Added the lowrank_gromov_wasserstein solver in __init__.py
  • Added the paper reference in READ.me
  • Created an example for Low Rank GW (plot_lowrank_GW.py) in examples/others

Motivation and context / Related issue

Adding a new low rank solver for gromov-wasserstein.
No existing issue is linked to this PR.

How has this been tested (if it applies)

Tests for each function are available in test_lowrank.py.
The Low Rank GW solver has also been tested by reproducing Figure 2 from the paper.

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.

laudavid and others added 30 commits October 24, 2023 14:54
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 97.19101% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 96.78%. Comparing base (81d7631) to head (a960b28).

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #614    +/-   ##
========================================
  Coverage   96.78%   96.78%            
========================================
  Files          81       83     +2     
  Lines       16142    16320   +178     
========================================
+ Hits        15623    15796   +173     
- Misses        519      524     +5     

@cedricvincentcuaz
Copy link
Collaborator

Hello @laudavid,
thank you for your nice PR.
Before I go into a detailed review of your code, could you please resolve conflicts with the current master branch and move low-rank GW functions to a new file ot/gromov/_lowrank.py and adjust your code accordingly ?

Best,
Cédric

@laudavid
Copy link
Contributor Author

Hello @laudavid, thank you for your nice PR. Before I go into a detailed review of your code, could you please resolve conflicts with the current master branch and move low-rank GW functions to a new file ot/gromov/_lowrank.py and adjust your code accordingly ?

Best, Cédric

Hello Cédric,
Thank you for your comment.
I resolved the conflits with the current master branch and moved lr GW to "ot/gromov/_lowrank.py".

Have a nice day,
Laurène.

Copy link
Collaborator

@cedricvincentcuaz cedricvincentcuaz left a comment

Choose a reason for hiding this comment

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

Thank you again for this nice PR :)
Here are some remarks to complete your work and clarify few elements.

examples/others/plot_lowrank_GW.py Outdated Show resolved Hide resolved
examples/others/plot_lowrank_GW.py Outdated Show resolved Hide resolved
examples/others/plot_lowrank_GW.py Outdated Show resolved Hide resolved
examples/others/plot_lowrank_GW.py Show resolved Hide resolved
ot/gromov/_lowrank.py Outdated Show resolved Hide resolved
ot/gromov/_lowrank.py Outdated Show resolved Hide resolved
ot/gromov/_lowrank.py Show resolved Hide resolved
ot/gromov/_lowrank.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/gromov/test_lowrank.py Outdated Show resolved Hide resolved
examples/others/plot_lowrank_GW.py Outdated Show resolved Hide resolved
examples/others/plot_lowrank_GW.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cedricvincentcuaz cedricvincentcuaz left a comment

Choose a reason for hiding this comment

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

Thank you for these updates :) The PR is ready imo, we recently fixed POT testing mechanism. Could you pull the last version of the master branch so that we can proceed with the merge ?

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

Successfully merging this pull request may close these issues.

None yet

2 participants