dynverse / dynmethods

A collection of 50+ trajectory inference methods within a common interface 📥📤
https://dynverse.org
Other
118 stars 26 forks source link

updated paga #77

Closed falexwolf closed 6 years ago

falexwolf commented 6 years ago

Hi Wouter and Robrecht,

this project is awesome! Really cool to have this highly comparable environment... We also cite your review in the revised PAGA preprint...

Here come a few changes to the PAGA container in the context of https://github.com/dynverse/dynmethods/issues/15. Things that might need further discussion:

Thank you for all your hard work and happy to discuss further. Alex

PS: I adapted standard Python coding style for the PAGA run.py... The only thing that is still unorthodox is the 2-space indenting... even though one should use 4-space indenting with Python, I didn't change it as it should probably be consistent across the whole repository

falexwolf commented 6 years ago

Just a couple of more comments 🙂

codecov[bot] commented 6 years ago

Codecov Report

Merging #77 into devel will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           devel     #77   +/-   ##
=====================================
  Coverage   0.52%   0.52%           
=====================================
  Files         34      34           
  Lines       2076    2076           
=====================================
  Hits          11      11           
  Misses      2065    2065
Impacted Files Coverage Δ
R/ti_container.R 0% <ø> (ø) :arrow_up:

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 6e22f7f...606596e. Read the comment docs.

zouter commented 6 years ago

Hi Alex Looks great! Really awesome that you're contributing to the project.

I'm fixing some small things now to the changes (eg. the parameter type of embedding type from string to discrete). Could you perhaps grant us the right to change your pull request (https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ ) so that we can push those changes?

I think in the current state, even though the pseudotime gets calculated, it will not be used in the output as it still outputs a "cluster_graph" instead of a trajectory. I will also try to fix this. Am I correct that for the calculation of the pseudotime the algorithm needs a root cell? Would it make sense to make this optional prior information and pick a random cell if no root cell is given? Or would prefer it to be a required prior information?

About the run times, you're right that this seems quite high. However, these values are now quite old and were calculated using way earlier versions of both the wrapper and PAGA (heck, we still use the name AGA :stuck_out_tongue: ). @rcannood is actually right now working on a more in depth scalability analyses, rather than a naive "average running time", so stay tuned!

The update homepage: :heart_eyes:

I will keep you updated!

falexwolf commented 6 years ago

Thanks!