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

[MRG] Implementation of two news algorithms: SaGroW and PoGroW. #275

Merged
merged 6 commits into from
Sep 17, 2021

Conversation

Hv0nnus
Copy link
Contributor

@Hv0nnus Hv0nnus commented Sep 6, 2021

Add two new algorithms to solve Gromov Wasserstein: Sampled Gromov Wasserstein and Pointwise Gromov Wasserstein. Based on Sampled Gromov Wasserstein (Machine Learning Journal 2021) (https://hal.archives-ouvertes.fr/hal-03232509/document).

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Motivation and context / Related issue

How has this been tested (if it applies)

It has been tested on some GW problems, with various values of hyper-parameters.

Checklist

  • The documentation is up-to-date with the changes I made. (I change few lines in README.md, comment the code and document it and provide some examples in examples/gromov/plot_gromov.py.)
  • I have read the CONTRIBUTING document.
  • All tests passed, and additional code has been covered with new tests. (I do not have changed the other tests, the deletions are only a typo correction: "constratints" to "constraints")

@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #275 (10df3f6) into master (96bf1a4) will decrease coverage by 0.27%.
The diff coverage is 85.60%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   93.34%   93.06%   -0.28%     
==========================================
  Files          17       17              
  Lines        3547     3679     +132     
==========================================
+ Hits         3311     3424     +113     
- Misses        236      255      +19     

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.

Hell @Hv0nnus and thank you very much for this PR.

I did a code review to ensure that the integration in POT and its documentation is seamless, there are a few things to do but I'm confident we can merge soon.

ot/gromov.py Outdated Show resolved Hide resolved
ot/gromov.py Show resolved Hide resolved
ot/gromov.py Outdated Show resolved Hide resolved
ot/gromov.py Show resolved Hide resolved
ot/gromov.py Show resolved Hide resolved
ot/gromov.py Outdated Show resolved Hide resolved
ot/gromov.py Outdated Show resolved Hide resolved
ot/gromov.py Outdated Show resolved Hide resolved
ot/gromov.py Show resolved Hide resolved
ot/gromov.py Show resolved Hide resolved
@rflamary rflamary changed the title [MRG] Implementation of two news algorithms: SaGroW and PoGroW. [WIP] Implementation of two news algorithms: SaGroW and PoGroW. Sep 8, 2021
@Hv0nnus
Copy link
Contributor Author

Hv0nnus commented Sep 13, 2021

After further check, I think the bug is not due to any of the modification of this branch but rather a bug on the test function "test_mapping_transport_class" on the file test_da.py. This bug occurs randomly, as shown on the .PNG. This bug should be probably not be solve on this branch and an issue should be raised. I test my branch and the main one, both have the same bug.
Rdm_bug_on_test_da

@rflamary
Copy link
Collaborator

Yes I agree, could you please open an issus with this? I think is should be corrected by setting the seed in the test but it should also be looked at because there is clearly a fail condition on this mapping estimation.

@Hv0nnus Hv0nnus changed the title [WIP] Implementation of two news algorithms: SaGroW and PoGroW. [MRG] Implementation of two news algorithms: SaGroW and PoGroW. Sep 17, 2021
@rflamary rflamary merged commit e0ba31c into PythonOT:master Sep 17, 2021
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.

2 participants