-
Notifications
You must be signed in to change notification settings - Fork 25
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
local plotting #159
local plotting #159
Conversation
Codecov Report
@@ Coverage Diff @@
## master #159 +/- ##
==========================================
- Coverage 87.11% 87.03% -0.09%
==========================================
Files 73 75 +2
Lines 5130 5521 +391
==========================================
+ Hits 4469 4805 +336
- Misses 661 716 +55
Continue to review full report at Codecov.
|
I'm really excited to take a look at this @Lioscro! One quick comment -- I think this PR will really benefit from a little notebook tutorial. What do you think? |
Of course! Keep in mind there are still a ton of things to add to this PR, so I'll get to that eventually. |
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.
Hi Joseph,
Amazing PR! The code is really clean and understandable. Thanks for this tremendous effort.
I had some clerical comments, but otherwise it looks really good.
Also, if I understand correctly, the plotting functions at the moment can only consume allele tables? I don't think it's in the scope of this PR, but perhaps we can add (and this is something I may be able to help on) functionality to consume character matrices as well?
Thanks!
Richard
I think that is a great idea! I'll open a new issue to implement this. I also considered doing that in this PR, but at the moment I just wanted to replicate all the important functionalities from iTOL plotting. |
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.
Wow! I am thoroughly impressed by this fantastic PR! I don't have anything I'd like to see changed per se, but before you merge this in, can you add these functions to the documentation website? I'm happy to help if you're unfamiliar with how our docs
folder works. For this reason, I'll Request Changes until we get the docs updated.
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.
Looks good to me Joseph! Thank you for clearing up all of my comments!
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.
This looks fantastic! Thanks so much for the hard work here. The only thing missing is the notebook tutorial on the documentation site. I'll add that in here for you.
Support for plotting trees locally, instead of submitting to iTOL (since this is paid now...)
Samples (3730_NT_T1)
Polar
Rectangle