[MRG] Sliced OT Plans#767
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #767 +/- ##
==========================================
+ Coverage 96.78% 96.81% +0.02%
==========================================
Files 118 124 +6
Lines 23891 24234 +343
==========================================
+ Hits 23122 23461 +339
- Misses 769 773 +4 🚀 New features to boost your workflow:
|
clbonet
left a comment
clbonet
left a comment
There was a problem hiding this comment.
Nice PR @eloitanguy @lchapel!
Here are few comments.
Co-authored-by: Clément Bonet <32179275+clbonet@users.noreply.github.com>
…into dev_sliced_plans
rflamary
left a comment
rflamary
left a comment
There was a problem hiding this comment.
Hello thanks @eloitanguy and @lchapel
the PR is looking very good, but i'm not convinced by the name of the main functions. since they all return plans they should have plan in the name. Final min pivot is not widely used and will not be understood by the users, I suggested something below but I'm open to suggestions (probably better to discuss on slack) ;).
| return plans, nx.stack(costs) | ||
|
|
||
|
|
||
| def min_pivot_sliced( |
There was a problem hiding this comment.
| def min_pivot_sliced( | |
| def min_lifted_sliced_plan( |
not sure about this one but min pivot is no clear enough for the users.
There was a problem hiding this comment.
lifted_sliced is another concept in our paper, maybe we could call this function min_pivot_sliced_plan?
| return plan, cost | ||
|
|
||
|
|
||
| def expected_sliced( |
There was a problem hiding this comment.
| def expected_sliced( | |
| def expected_sliced_plan( |
4 participants
Types of changes
Note: this PR continues #757 which was merged by accident.
Implements min-Pivot Sliced Plans Mahey et al., Tanguy et al. and Expected Sliced Plans Liu et al., Tanguy et al., and Generalised Sliced Wasserstein Plans Chapel et al..
Checklist:
RELEASES.mdandREADME.mdtforjax, test accordingly, and add a warning in the docPR checklist