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] (f)gw barycenter solvers new features and cleaning #578

Merged
merged 8 commits into from
Nov 21, 2023

Conversation

cedricvincentcuaz
Copy link
Collaborator

@cedricvincentcuaz cedricvincentcuaz commented Nov 20, 2023

Types of changes

  • add a new option to (un)regularized (f)gw barycenter solvers : conv_criterion taking values into {"barycenter", "loss"} so the user can choose convergence criterions either based on the absolute norm variations of the barycenter, or relative loss variations, respectively : The last criterion is most likely meaningful for most applications, plus suit to the ones used in inner (f)gw solvers based on conditional gradient. This feature is also added to the entropic (f)gw barycenter solvers and would come with an overhead of computing the loss at the last iteration of the inner entropic solvers. Whereas for CG inner functions, computing the barycenter loss is performed in O(#samples).

  • Trying to harmonise the behavior across all (f)gw barycenter solvers :

    • Some solvers were updating their convergence criterions at only few epochs to reduce the computational burden of computing norms over the barycenter, but it is really not convenient to analyse the convergence of these solvers.
    • Add fixed_structure and fixed_features to entropic_fused_gromov_barycenter, same than unregularized solver.
  • Adapt and improve these functions documentation.

Motivation and context / Related issue

How has this been tested (if it applies)

  • Previous tests are running. Additional tests to do.

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 20, 2023

Codecov Report

Merging #578 (a35211b) into master (31455e5) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
+ Coverage   96.61%   96.64%   +0.03%     
==========================================
  Files          74       74              
  Lines       15037    15187     +150     
==========================================
+ Hits        14528    14678     +150     
  Misses        509      509              

if conv_criterion == 'barycenter':
log_['err_feature'] = []
log_['err_structure'] = []
log_['Ts_iter'] = []
Copy link
Collaborator Author

@cedricvincentcuaz cedricvincentcuaz Nov 20, 2023

Choose a reason for hiding this comment

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

I am really not convinced that Ts_iter is useful to anyone, knowing that it uses a lot of memory relatively ^^'

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 PR. I think we shoumd call it stop_criterioninstead of conv it is mor eclear to me WDYT?

ot/gromov/_bregman.py Outdated Show resolved Hide resolved
ot/gromov/_bregman.py Outdated Show resolved Hide resolved
ot/gromov/_bregman.py Outdated Show resolved Hide resolved
@rflamary rflamary changed the title [WIP] (f)gw barycenter solvers new features and cleaning [MRG] (f)gw barycenter solvers new features and cleaning Nov 21, 2023
RELEASES.md Outdated Show resolved Hide resolved
@rflamary rflamary merged commit cffb6cf into PythonOT:master Nov 21, 2023
13 checks passed
@cedricvincentcuaz cedricvincentcuaz deleted the fgw_bary branch January 17, 2024 01:03
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