-
Notifications
You must be signed in to change notification settings - Fork 9
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
Documentation/tutorial #42
Conversation
* Add basic config files * Add basic docs * Add linting CI action
* Add alpha version * Fix FusedGromow * [ci skip] Fix FGW * Fix FGW v2 * Polish a few things * Add tests * Add tests, CI included * Add notebook comparison with gt FGW * Add docs * Add numpy to requirements * [ci skip] Skip 2 jobs to reduce credit usage * Address comments * Rename package * Fix not passing epsilon, add novosparc
* Update the Readme * [ci skip] Add logo Co-authored-by: Michal Klein <michal.klein@protonmail.com>
* Print losses as from POT in FGW * Print also tau
* Fix tau, remove novosparc option * Remove artifact
* Fix tau, remove novosparc option * Fix eps/None, update logging, jit=True * Parametrize test by jitting * Fix jitting sinkhorn instead GW * Try fixing test * Fix not passing kwargs to test
* Fix tau, remove novosparc option * Fix eps/None, update logging, jit=True * Add converged property * Parametrize test by jitting * Use rtol/atol for FGW convergence, rename n_iters * Fix typo in test
* Add first comparison of GW with LineageOT * Add benchmarking notebook * Update bench notebook * Add bench notebooks, plotting notebook * Add first version of tedsim * Add C.elegans notebook * Update LOT bench notebook * Update notebooks * Remove old plotting notebook * Fix Geometry, add POT impl. * Change loops * Fix typo * Try out Cassiopeia * Add LOT barcode notebook plot * Add LOT barcode notebook plot fix * Add fitted tree distances notebook * Fix not propagating eps to regOT * Create new Geometry * Fix test
* Add FGW term scaling * Remove redundant scale passing
* Add random init for FGW * Fix typing for 3.7
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
* Add warning if not converged * Add check for marginals * Fix tests * Update error msg
* Add first comparison of GW with LineageOT * Add benchmarking notebook * Update bench notebook * Add bench notebooks, plotting notebook * Add first version of tedsim * Add C.elegans notebook * Update LOT bench notebook * Update notebooks * Remove old plotting notebook * Fix Geometry, add POT impl. * Change loops * Fix typo * Try out Cassiopeia * Add LOT barcode notebook plot * Add LOT barcode notebook plot fix * Add fitted tree distances notebook * Add LOT/cassiopeia comparison * Update LOT bench notebook * Fix typo * Update C. elegans notebook v2 * Add convergence notebook * Update convergence notebook * Update convergence notebook v2 * Add convergence bifurcation plot * Add C. elegans plot notebook * Add C. elegans UMAP * Use color scaling as in LineageOT * Update convergence notebooks * Add working TedSim notebook * TedSim generate datasets * Update TedSim ranges * Add LOT seeded notebok * Add LOT seeded notebook v2 * Add forgotten scale * Add C. elegans CellRank skeleton notebook * Run c elegans analyis * Add adata creation notebook * Fix name Co-authored-by: Marius Lange <marius.lange@t-online.de>
* Add warning if not converged * Add check for marginals * Fix tests * Update error msg * Fix jitting, FusedGW not passing kwargs
@@ -0,0 +1,509 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also enable 64-bit and add a sentence below why we do it (with 32-bit, sometimes the precision wasn't enough).
Reply via ReviewNB
@@ -0,0 +1,509 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #4. module_path = os.path.abspath(os.path.join('..'))
The local import seems unnecessary in this notebook, would remove it if that's the case.
Reply via ReviewNB
@@ -0,0 +1,509 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typos + maintain consistency (would start sentences capitalized + end with .
)
Reply via ReviewNB
@@ -0,0 +1,509 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add link to the paper.
Furthermore, I'd format both notebooks using black (there should be some extension, can't remember its name).
Reply via ReviewNB
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would remove this cell and run below benchmark_moscot(sim, alpha=0, epsilon=epsilon)
- just adds extra cell with little info.
Also applied for the other alphas (1, and 0.9).
Reply via ReviewNB
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would all params (in this case scale
) to one place (e.g. where you define epsilon
Reply via ReviewNB
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, not sure how informative this is, because of overplotting/the data.
To my eyes, OT, LOT and us look the same.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in terms of comparison; hence added the cost but thought it was beneficial to show how one can visualize easily the obtained mappings/. Am open for suggestions for better visuals here.
@@ -0,0 +1,715 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version has a after incorporation of @michalk8 comments |
Are our results in the simple 1D example correct? In the original POT example, the selling point was that FGW is a best-of-both-worlds scenario: it preserves feature relations while also enforcing structural correspondence (Fig. 2). But that's not what we are showing (Fig. 1 below) - in our results, FGW looks just super similar to GW and the user if left wondering what the added benefit really is. We need to make this example more convincing. Also, at the moment, it's a bit confusing because x and y have different meanings throughout the notebook - they're at the same time the corresponding feature spaces for source and target, while also the x and y axis in the plots we show. I know this is also the case in the original POT tutorial, but I also find this confusing there. We should just improve the naming a bit. Also, I don't get why the features have two different names (a and b) - isn't the whole story here that features can be compared across spaces? So it should be the same feature we're looking at, no? On the other hand, the structural coordinates should have different names, maybe just not x and y, because that get's confusing. Also, when we shot the final couplings, it's a bit hard to relate this to what we had earlier because suddenly, there are Fig. 1 (ours)Fig. 2 (POT) |
For a tutorial to be convincing, it just has to be super obvious - at the moment, we leave a lot of work on the users side to try and figure out what we're trying to sell here. We need to make this much clearer by improving nomenclature, descriptions and basically making sure our results also show a best-of-both-worlds scenario. Also, most users that come won't really understand what these two "extremal regimes" are, we need to add some explanations. |
Also we need to give credit to the original POT example, not just to Vayer. |
I did some minor edits on the text, but this needs some more work. |
Thanks Zoe! Using our |
Other than that, did you have a chance to incorporate my suggestions yet? Let me know once you do, happy to go over this again. |
I am afraid my main problem is that its not super clear to me what do we add on POT's tutorial here .. |
I am sure there is; will manually look for it |
@zoepiran I had to close this PR to delete an old branch ( |
Hi @Marius1311,
added the tutorials - hope it follows your line of thought.
(a) showing only the 'true tree' setting - I think it passes the point and this way we can refrain from forcing users only going through tutorial to install Cassipeia (and Gurobi etc.).
(b) thought it is useful and understandable to use the benchmarking functions (rather than breaking them line by line) hence some of the explanations are maybe not as detailed.