Closed zsteve closed 2 years ago
Sorry for not replying earlier, I just came back from vacation :slightly_smiling_face:
One thing I will note here is that quite a few parts of
solve!
can be reused betweenSinkhornGibbs
andSinkhornBarycenterGibbs
-- essentially the differences come down to the update steps and the arguments passed to the convergence check. One solution here could be to wrap the update steps into algorithm-specific calls via multiple dispatch. I haven't attempted this yet (maybe a future PR?) but I'd be keen to hear your thoughts.
If this is possible, it could be a good way to reduce code and ensure that the implementations are consistent. However, I think this can be left for a future PR.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
src/entropic/sinkhorn_barycenter.jl | 49 | 50 | 98.0% | ||
<!-- | Total: | 69 | 70 | 98.57% | --> |
Totals | |
---|---|
Change from base Build 1158342145: | 0.5% |
Covered Lines: | 588 |
Relevant Lines: | 608 |
Merging #110 (6e14eda) into master (5354fb6) will increase coverage by
0.45%
. The diff coverage is98.57%
.
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
+ Coverage 96.25% 96.71% +0.45%
==========================================
Files 11 12 +1
Lines 561 608 +47
==========================================
+ Hits 540 588 +48
+ Misses 21 20 -1
Impacted Files | Coverage Δ | |
---|---|---|
src/entropic/sinkhorn_barycenter.jl | 98.00% <98.00%> (+6.69%) |
:arrow_up: |
src/entropic/sinkhorn_barycenter_gibbs.jl | 100.00% <100.00%> (ø) |
|
src/utils.jl | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 5354fb6...6e14eda. Read the comment docs.
Hi all,
I've done a quick refactor of
sinkhorn_barycenter
to now conform to conventions introduced in #100 (which I'm sorry I missed at the time due to moving), reusing code fromsinkhorn.jl
andsinkhorn_gibbs.jl
where possible.Problem parameters are specified in
SinkhornBarycenterSolver
, and the standard scaling algorithm corresponds to settingalg = SinkhornBarycenterGibbs()
I've had to define
SinkhornBarycenterGibbsCache
separately because in order to do the convergence checks, I need to keep track of the barycenter at each iteration, nameda
in the code. For the convergence check I use the existingSinkhornConvergenceCache
struct.Let me know if there's anything further that can be cleaned up in the code.
One thing I will note here is that quite a few parts of
solve!
can be reused betweenSinkhornGibbs
andSinkhornBarycenterGibbs
-- essentially the differences come down to the update steps and the arguments passed to the convergence check. One solution here could be to wrap the update steps into algorithm-specific calls via multiple dispatch. I haven't attempted this yet (maybe a future PR?) but I'd be keen to hear your thoughts.