-
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
[WIP] Add debiased barycenter (Sinkhorn + convolutional sinkhorn) #291
Conversation
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.
just a first set of comments
please pay attention to public API
ot/bregman.py
Outdated
@@ -1081,6 +1051,11 @@ def barycenter(A, M, reg, weights=None, method="sinkhorn", numItermax=10000, | |||
numItermax=numItermax, | |||
stopThr=stopThr, verbose=verbose, | |||
log=log, **kwargs) | |||
elif method.lower() == 'debiased': | |||
return barycenter_sinkhorn_debiased(A, M, reg, weights=weights, |
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.
so barycenter_sinkhorn_debiased is also a new public function?
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.
should we put it here? It is not another algorithm to solve the baycenter, it solve a different problem with a diferent solution.
Maybe we should use a debiased parameter in the new API but for now a new public function seems OK.
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.
since it's a new function, and the default 'method' is 'sinkhorn', UX won't change if we make the specific methods funcs private, it's done in the last push.
Codecov Report
@@ Coverage Diff @@
## master #291 +/- ##
==========================================
+ Coverage 93.29% 93.37% +0.08%
==========================================
Files 21 21
Lines 4502 4799 +297
==========================================
+ Hits 4200 4481 +281
- Misses 302 318 +16 |
pushed some stuff on example.
I would need to do a pass on bregman.py maybe too
… |
Jax doesn't support item assignment in arrays which is needed in sinkhorn_log functions (for e.g here https://github.com/hichamjanati/POT/blob/5297495058d662a5f4eee779c5b33dd20343cb30/ot/bregman.py#L1499) to avoid using lists. any obvious workaround for this ? I'm thinking adding a method |
barycenters are done with a relatively small number of distributions in practice so using lists is not much of a problem here (you can stack after the loop). I agree this is a pain so either you use list (maybe test to see the overhead in practice but it should be small) or you raise a not implemented for jax. |
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.
This is great but we have a bade coverage due to all the parameters. could you put verbose=True on some test and also test for the warning when numItermax is very small?
Types of changes