More sophisticated convergence checks in sinkhorn#79
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Pull Request Test Coverage Report for Build 883838094Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Tests pass reliably now at least in this PR. However,
got me thinking that maybe the 1-norm would be a bit more intuitive and/or has a nice motivation. So I switched to the 1-norm, let's see if tests still pass. |
davibarreira
left a comment
davibarreira
left a comment
There was a problem hiding this comment.
Just left a small comment about the docstring.
| Every `check_convergence` steps a convergence check of the error of the marginal | ||
| `μ` with absolute tolerance `atol` and relative tolerance `rtol` is performed. The default | ||
| `rtol` depends on the types of `μ`, `ν`, and `K`. After `maxiter` iterations, the | ||
| computation is stopped. |
There was a problem hiding this comment.
Perhaps make clearer what is the atol and the rtol. By just reading the docstring, I cannot figure out exactly how these parameters are affecting the model, specially rtol, which I'm not acquainted with.
There was a problem hiding this comment.
Hmm, I'd like to avoid discussing internal variables of the algorithm. It's the same principle as in isapprox, maybe it is sufficient to refer to it? Or would you prefer a more detailed explanation? The docstrings already feel a bit (too) long but maybe this is mainly since some of the information is redundant and there are too many different functions.
There was a problem hiding this comment.
Personally, as a user, I'd prefer to have it clearly in the docstring. But I do understand if you rather leave it out. Yes, I agree that perhaps we have too much redundant information.
Well, I was thinking of saying explicitly how mu convergence is considered, like you wrote in the PR about using 2-norm.
There was a problem hiding this comment.
I also added some more explanations here.
| Every `check_convergence` steps a convergence check of the error of the marginal | ||
| `μ` with absolute tolerance `atol` and relative tolerance `rtol` is performed. The default | ||
| `rtol` depends on the types of `μ`, `ν`, and `C`. After `maxiter` iterations, the | ||
| computation is stopped. |
There was a problem hiding this comment.
The same as the comment above. Perhaps we could make it clearer how the atol and rtol are used.
| sinkhorn( | ||
| μ, ν, C, ε; atol=0, rtol=atol > 0 ? 0 : √eps, check_convergence=10, maxiter=1_000 | ||
| ) | ||
|
|
There was a problem hiding this comment.
I missed this in the first read, but why atol = 0 as the default? Why not 1e-9 as before? Also, is the eps in sqrt(eps) the ε in the argument? If so, then I thinks it's clearer to have \\sqrt(ε).
There was a problem hiding this comment.
Absolute tolerances depend on the scale and hence usually are meaningless. The values are the default values of isapprox which also contains an example that motivates this and explains why one should use isapprox(x, y) instead of isapprox(x - y, 0). The eps is the machine epsilon of the floating point type (that depends on mu, nu, and C, as explained in the docstring). Also the notation with √eps is copied from isapprox. I thought it would be OK since it is part of base Julia.
There was a problem hiding this comment.
Absolute tolerances depend on the scale and hence usually are meaningless.
IMO even more so with the inf-norm since for vectors of increasing dimension n even vectors with increasingly small inf-norm of c/n, where c is a constant, can have a constant 1-norm of c.
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 84.95% 85.93% +0.97%
==========================================
Files 1 1
Lines 246 256 +10
==========================================
+ Hits 209 220 +11
+ Misses 37 36 -1
Continue to review full report at Codecov.
|
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…nsport.jl into dw/sinkhorn_tolerances
|
I give up, it seems there are no guarantees and the test failures might reappear on MacOS at any time. The strange thing is that all started when we switched from pip to Conda.jl, so I wonder if there is some problem with the POT version from Conda on MacOS. I don't want to spend more time on this, so I just disable tests on MacOS. In principle, I think it would be sufficient to only run them with Linux since we do not use any os-specific features but Windows seems to be more predictable so I did not remove it. |
Agreed. |
4 participants
I started to debug the spurious test errors on MacOS in #45 but I think it should really be done in a separate PR (so I'll revert the corresponding changes in the other PR).
I think one (the main?) reason for the differences between our and POT's results is that we use a different convergence check. POT checks if the 2-norm of the difference for the target marginal is below the given threshold whereas currently we check if the inf-norm of the difference for the source marginal is below the given threshold. I don't think it matters if one uses the source or target marginal (it really depends on how the order of the iterations are defined, if one starts with u or with v) but I think the choice of the norm does make a difference. Of course, all standard norms such as 1-norm, 2-norm, and inf-norm are equivalent but their bounds on each other scale with the number of dimensions, so depending on the size of the histograms one would have to scale the thresholds quite a bit to guarantee that the error is at most as large as in the other norm.
Additionally, I assume it can be useful to be able to specify both a relative and absolute tolerance. Maybe not so much for
sinkhornif everything is normalized to 1-norm of 1 but e.g. in the unbalanced case where we (and POT) base the convergence check on the distance between subsequent scaling factor iterates.In the first draft of this PR I just went with the 2-norm and the same tolerances that are used by
isapproxto be a bit consistent.On a side note, in the COT book it is explained in remark 4.14 how these checks of the marginals can be motivated theoretically. COT uses the 1-norm as an example, I guess this could be interesting as well since it provides a clear bound on how much the mass deviates from the ideal of 1 (or whatever way the marginals are scaled).