Support rectangular (and arbitrarily configurable) layouts; minor typo fixes / detail additions in CLI #145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also removed some unused code (the PARAMETERS stuff)
python side of things is mostly worked out, just need to adjust igraph creation to use ints instead of node names then fix scaling stuff and we should at least have the positions set out. from there it's a matter of modifying js + tweaking things to work well
This was a problem due to trees with duplicate node names (apparently the rooted tree you get from the Q2 moving pictures tutorial has a bunch of duplicated names). Adjusted tests accordingly -- also now the to_igraph() test is a lot more elegant (much less hardcoded data), which is great.
This reverts commit 077a8d6.
Inspired by what TopiaryExplorer and iTOL do. Much simpler, though.
shouldn't actually make a difference in output visualization, since everything is scaled by the tree height anyway
This has the added bonus of making the code cleaner, etc. This makes the "igbug" and "tiny" test trees demonstrably work as expected -- i'll try to convert those into actual tests soon
also use leafcount rather than calling .count(tips=True)
(Need to actually transmit multiple layouts' data to the JS, then make the JS do something when you change the radio buttons...)
All that's left to get rectangular layouts done is just adding the vertical bars when drawing the tree with that layout. (... Also, adding tests and documentation. That will be important.)
preserves stuff like sample colors
NOTHING else in the Empress Python or JS code makes any assumptions about the layout algorithms. this should make adding / removing layouts pretty painless
to demonstrate the ease with which you can add new layouts to Empress now :D
Still needs testing, double-checking, etc. but this is great also this now centers nodes over children rather than tips
Uses @ElDeveloper's suggestion, thanks! Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>
thonk thanks @ElDeveloper for pointing this out :)
... for now >:)
|
OK, I think the requested changes so far should be mostly addressed. Please let me know if there's anything else you'd like me to change! Thanks guys! |
kwcantrell
reviewed
Mar 31, 2020
kwcantrell
reviewed
Mar 31, 2020
kwcantrell
reviewed
Mar 31, 2020
kwcantrell
reviewed
Mar 31, 2020
kwcantrell
reviewed
Mar 31, 2020
kwcantrell
reviewed
Mar 31, 2020
|
This looks good! I added a few comments but nothing major. |
|
@kwcantrell, if all looks good to you feel free to merge when your comments are addressed. |
Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>
Suggestion from @kwcantrell
This was referenced Apr 1, 2020
Closed
|
I think I've addressed all of the comments by this point, but please either of you let me know if you'd like me to make any more changes :) |
ElDeveloper
approved these changes
Apr 4, 2020
|
@kwcantrell if all looks good to you feel free to merge this. |
|
Great work everyone! |
|
Thanks both! |
This was referenced Apr 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Screenshot on the tree
(((a:1,e:2)f:1,b:2)g:1,(c:1,d:3)h:2)i:100;, where I configured things so that all tips belong to the same sample category:There are a lot of changes in this PR (some of them are a lot simpler than they seem -- e.g. a bunch of JS files got slightly changed because the version of
prettierTravis was using changed, and I had to rerun it on the JS files to get the builds to pass), so it might be easiest for us to walk through the PR together next week.Thanks!