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] Fix order problems in (F)GW barycenters and utils #576

Merged
merged 8 commits into from Nov 20, 2023

Conversation

cedricvincentcuaz
Copy link
Collaborator

@cedricvincentcuaz cedricvincentcuaz commented Nov 15, 2023

Types of changes

  • detail and fix operations in gromov.update_square_loss and gromov.update_kl_loss.
  • fix order of operations in (f)gw solvers using CG and entropic solvers.

Motivation and context / Related issue

How has this been tested (if it applies)

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.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #576 (b7179ad) into master (ef6c3c1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #576   +/-   ##
=======================================
  Coverage   96.61%   96.61%           
=======================================
  Files          74       74           
  Lines       15036    15037    +1     
=======================================
+ Hits        14527    14528    +1     
  Misses        509      509           

@cedricvincentcuaz cedricvincentcuaz changed the title [WIP] Fix order problems in (F)GW barycenters and utils [MRG] Fix order problems in (F)GW barycenters and utils Nov 15, 2023
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.

Nice @cedricvincentcuaz , a few comments before merging.

ot/gromov/_bregman.py Show resolved Hide resolved
ot/gromov/_utils.py Outdated Show resolved Hide resolved
ot/gromov/_utils.py Outdated Show resolved Hide resolved
ot/gromov/_utils.py Outdated Show resolved Hide resolved
@@ -1454,7 +1463,7 @@ def test_fgw_barycenter(nx):

with pytest.raises(ot.utils.UndefinedParameter): # to raise warning when `fixed_structure=True`and `init_C=None`
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a warning but and error please

@rflamary rflamary merged commit 31455e5 into PythonOT:master Nov 20, 2023
15 checks passed
@cedricvincentcuaz cedricvincentcuaz deleted the gw_bary branch November 20, 2023 17:33
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