[MRG] (f)gw barycenter solvers new features and cleaning by cedricvincentcuaz · Pull Request #578 · PythonOT/POT

Conversation

@cedricvincentcuaz

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.
…rmonise these solvers a bit

@codecov

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              

cedricvincentcuaz

if conv_criterion == 'barycenter':
log_['err_feature'] = []
log_['err_structure'] = []
log_['Ts_iter'] = []

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 ^^'

rflamary

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?

@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

rflamary

2 participants

@cedricvincentcuaz @rflamary