-
Notifications
You must be signed in to change notification settings - Fork 31
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
Ultrametric option #444
Merged
Merged
Ultrametric option #444
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
3acf88a
ENH add function for computing ultrametric lengths
gwarmstrong 51c9dd2
ENH make getUltraMetricLengths correspond to tree index
gwarmstrong 9ddcb75
MAINT refactor layouts to take arbitrary length getter
gwarmstrong acb58c7
DOC include docstring argument for getLength
gwarmstrong cdf4426
STY make jsstye for ultrametric
gwarmstrong 4f5fbf8
DOC add explanation of ultrametric algorithm
gwarmstrong 4dd3bd6
Auto stash before rebase of "refs/heads/ultrametric-option"
gwarmstrong 6f175cf
FIX lengthGetter pass into circularLayout
gwarmstrong 8c36568
ENH add radio button and improve logic for branch length choice
gwarmstrong 5b07e7e
FIX some comments
gwarmstrong 61ada1e
ENH fix html stuff from marcus code review
gwarmstrong 22093af
ENH add section for clade sorting
gwarmstrong 9798a0a
ENH remove red from branch lengths warning
gwarmstrong 27ce1f0
ENH change caps
gwarmstrong 867eb96
ENH hide branch length warning when not in use
gwarmstrong cd13f59
ENH replace deteremine lenghs text
gwarmstrong 4f94728
MAINT jsstyle for branch methods
gwarmstrong 054de76
DOC add comment for _determineLengthGetter
gwarmstrong c4801e5
TST ensure ultrametric tree stays same
gwarmstrong fd037c4
MAINT style on test
gwarmstrong 8020902
ENH clarify logic for determining branch lengths
gwarmstrong 3656869
DOC more specific comments
gwarmstrong ae35490
INT change interface display of branch lengths
gwarmstrong e56dc7c
Update empress/support_files/js/side-panel-handler.js
gwarmstrong fdff5e4
MAINT remove ignoreLengths argument to layout functions
gwarmstrong 32df93f
Merge branch 'ultrametric-option' of https://github.com/gwarmstrong/e…
gwarmstrong d492fed
MAINT tests style
gwarmstrong 4b83692
Update empress/support_files/templates/side-panel.html
fedarko b9759ed
Update empress/support_files/js/side-panel-handler.js
fedarko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
Yeah, that's fair. My take on this is that I think figuring out lengths junk should be the job of the stuff in LayoutsUtils, just so we can avoid having logic in the main
Empress
class as much as we can (since it's already probs too big for its own good, and also to make testing easier).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 think that passing a function for the lengths is nice because it opens the door for us to do some pretty cool stuff later, without having to make modifications to the
LayoutsUtil
module that would make those functions signatures more unmanageable.E.g., say we wanted a feature where you could adjust branch lengths on the fly, you could define some function like
And it shouldn't really be
LayoutsUtil
's job to support this.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.
What if we moved the logic from above into an
Object
of predefined length getters that lives inLayoutsUtil
but leave theLayout
functions extensible by functions.E.g., this block would look something more like
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.
Let me know if this commit 8020902 resolve this.
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.
Whoops, sorry for taking so long to get back to you on this. Yeah I can definitely see the utility of defining this behavior with functions -- I think mainly I was just hesitant to add more stuff to
Empress
. I like 8020902's solution to this a lot; I think having this as a function that lives inLayoutsUtil
(or at least somewhere that is outside ofEmpress
and outside of each individual layout function inLayoutsUtil
) gets us the best of both worlds.I think this is basically resolved, IMO, although I do think that now that we're making this change we should probably bite the bullet and remove the
ignoreLengths
parameters. If it's possible it'd be nice to do that in this PR (just to avoid confusion with having these redundant arguments still existing in the codebase -- I don't think deprecation is necessary since AFAIK no one is really out here relying on Empress' APIs), but if you think it'd be too much of a pain I'm cool to open a new issue for it.