Add a tuto about sparse monge displacements #374
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #2. output = sinkhorn.Sinkhorn()(linear_problem.LinearProblem(geom))
maybe worth jitting one solver for the entire NB, and reusing it?
Reply via ReviewNB
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. transported = dual_potentials.transport(x)
both in this example and that below, it would be great to draw a fresh sample for x, to showcase the out of sample property of the estimator
Reply via ReviewNB
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
most particles
"the same coordinate" -> since some points have more than one sometimes, "the" is a bit ambiguous
Reply via ReviewNB
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
here the "\tau" in "h_\tau" does not make much sense? maybe write directly \tau = ? or h = ?
Reply via ReviewNB
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
==========================================
+ Coverage 88.43% 88.49% +0.05%
==========================================
Files 51 52 +1
Lines 5605 5651 +46
Branches 836 837 +1
==========================================
+ Hits 4957 5001 +44
- Misses 529 530 +1
- Partials 119 120 +1 |
|
All your comments have been taken into account. WDYT now? |
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
The math doesn't completely render in the notebook.
In general, this notebook lacks some subsections, would add some.
Reply via ReviewNB
There was a problem hiding this comment.
where does the math not render here?
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #3. if axe is None:
Why is this called "axe"? Would rename to "ax", as this is standard in matplotlib naming conventions.
Reply via ReviewNB
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
Please add a link to the Sinkhorn class as {class}~ott.solvers.linear.sinkhorn.Sinkhorn.
Would also add {class}problem <ott.problems.linear.linear_problem.LinearProblem>
and {class}entropic map <ott.problems.linear.potentials.EntropicMap>
Reply via ReviewNB
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
Please add a link to the DualPotentials class as {class}dual potentials <ott.problems.linear.potentials.DualPotentials>
Also please add a link to the transport method (using {meth}).
Reply via ReviewNB
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,428 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I guess that is a bit too much for this tutorial? I don't even know it myself
There was a problem hiding this comment.
it's O(kd) :) i think it would make sense giving this complexity, and mention this starts having impact in higher dimensions (but not low d, as in this example), or removing the comment altogether because it's a bit too vague.
|
There's a small typo in the notebook, that's why linter fails. After addressing the last few of my comments, we can merge! |
* update index * init nb * add paper * jit solver * add new source points * rephrase * fix h_tau * add samples description * fix from michal's comments * fix math * fix typo * Update links to functions --------- Co-authored-by: Michal Klein <[email protected]>
This adds a tuto demoing sparse monge displacements with mixed costs.