Skip to content
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

Introduce primal_cost and dual_cost for Sinkhorn outputs (only primal for LR) for arbitrary geometries. #184

Merged
merged 13 commits into from
Nov 28, 2022

Conversation

marcocuturi
Copy link
Contributor

@marcocuturi marcocuturi commented Nov 24, 2022

  • Solves Define a ot_cost property for sinkhorn and sinkhorn_lr outputs #180 by introducing a property to compute primal_cost, the bare cost of transportation (i.e. the dot product between a coupling matrix and the corresponding cost matrix) for sinkhorn and sinkhorn_lr (it was already there for the latter, just renamed). While trivial for standard geometries, notably generic Geometry objects, this is a bit more complicated for PointCloud and required the ability to cast an arbitrary Grid (itself with possibly low rank costs at each dimension) into a low-rank geometry.
  • Because casting Grid to LRCGeometry requires casting all cost matrices into LRC, introduce the possibility, usingrank=0 or rank>=min(n,m) to default to SVD to cast an exact, low-rank factorization.
  • Introduce dual_cost (much easier to compute) for outputs of sinkhorn and sinkhorn_lr.
  • While testing for primal_cost in Grid, exhibit bug Grid geometry handling of 0 weights seems buggy #185.
  • Correct stopping criterion in balanced sinkhorn to amend for the fact that in the case updates are parallel, both marginals should be computed.
  • As a result, modify the computation of symmetric terms in sinkhorn divergence computations as using parallel updates by default, and test this leads to improved or equivalent computational times.

@marcocuturi marcocuturi changed the title instantiate ot_cost for arbitrary geometry. instantiate ot_cost for arbitrary geometry. Nov 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022

Codecov Report

Merging #184 (1f2104a) into main (156ef2b) will decrease coverage by 0.01%.
The diff coverage is 97.24%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
- Coverage   89.28%   89.26%   -0.02%     
==========================================
  Files          51       51              
  Lines        5187     5234      +47     
  Branches      527      529       +2     
==========================================
+ Hits         4631     4672      +41     
- Misses        430      434       +4     
- Partials      126      128       +2     
Impacted Files Coverage Δ
ott/tools/sinkhorn_divergence.py 98.24% <ø> (+3.60%) ⬆️
ott/geometry/low_rank.py 96.42% <66.66%> (-0.66%) ⬇️
ott/solvers/linear/sinkhorn_lr.py 96.25% <83.33%> (-0.34%) ⬇️
ott/geometry/geometry.py 92.15% <97.29%> (-0.10%) ⬇️
ott/geometry/graph.py 94.40% <100.00%> (+0.03%) ⬆️
ott/geometry/grid.py 86.66% <100.00%> (-4.02%) ⬇️
ott/geometry/pointcloud.py 85.54% <100.00%> (+0.25%) ⬆️
ott/solvers/linear/sinkhorn.py 97.85% <100.00%> (-0.29%) ⬇️
ott/solvers/quadratic/gromov_wasserstein.py 93.05% <100.00%> (ø)
... and 3 more

@marcocuturi marcocuturi requested a review from michalk8 November 25, 2022 08:38
@michalk8 michalk8 added the enhancement New feature or request label Nov 25, 2022
@marcocuturi marcocuturi requested a review from michalk8 November 25, 2022 22:53
@marcocuturi marcocuturi changed the title instantiate ot_cost for arbitrary geometry. Introduce primal_cost and dual_cost for Sinkhorn outputs (only primal for LR) for arbitrary geometries. Nov 27, 2022
@marcocuturi
Copy link
Contributor Author

marcocuturi commented Nov 27, 2022

Changed my mind, ot_cost was a bit ambiguous and clearly overloaded. I prefer primal_cost (and dual_cost), which are (a bit) more explicit. primal_cost will always be the linear term of a linear OT problem (or, by extension, in a future PR, the quadratic/fused for a quadratic/fused pbm).

@marcocuturi marcocuturi merged commit c00bc12 into main Nov 28, 2022
@marcocuturi marcocuturi deleted the marco-main branch November 28, 2022 10:43
michalk8 pushed a commit that referenced this pull request Jun 27, 2024
…imal for LR) for arbitrary geometries. (#184)

* instantiate ot_cost for arbitrary geometry.

* lint

* fix assert in rank for to_LRCGeometry method.

* fixes

* linter

* linter

* fix extra bit of memory.

* _check_LRC_dim becomes private.

* bug

* linters and comments.

* doc

* change naming, introduce dual_cost

* linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants