-
Notifications
You must be signed in to change notification settings - Fork 506
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] New API for gromov solvers #536
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #536 +/- ##
==========================================
+ Coverage 96.31% 96.37% +0.06%
==========================================
Files 67 67
Lines 14136 14389 +253
==========================================
+ Hits 13615 13868 +253
Misses 521 521 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @rflamary,
thank you for this wonderful API for gromov solvers ! ;)
I've made a few comments to help you with the final details.
T, logv = entropic_fused_gromov_wasserstein( | ||
M, C1, C2, p, q, loss_fun, epsilon, symmetric, alpha, G0, max_iter, | ||
tol, solver, warmstart, verbose, log=True, **kwargs) | ||
|
||
logv['T'] = T | ||
|
||
lin_term = nx.sum(T * M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could avoid this overhead by defining both logv['quad_loss'] and logv['lin_loss'] directly in entropic_fused_gromov_wasserstein if log=True
Types of changes
in this PR I propose the implementation of the new API for Gromov-Wasserstsein solvers. Similarly to
ot.solve
we now have a functionot.solve_gromov
that can be used as followsMotivation and context / Related issue
How has this been tested (if it applies)
PR checklist