-
Notifications
You must be signed in to change notification settings - Fork 77
Allow mutations to be plotted per edge not per tree #1258
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1258 +/- ##
==========================================
+ Coverage 93.77% 93.78% +0.01%
==========================================
Files 26 26
Lines 21939 21991 +52
Branches 971 987 +16
==========================================
+ Hits 20573 20625 +52
Misses 1325 1325
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
90c300d
to
2dc4fac
Compare
Will review later - just a quick comment to ask if we are at the point where a legend might be helpful? |
Thanks @benjeffery. No hurry on this. Personally, I don't think we are quite at that point yet. The various fancy graphics I've added aren't always very suited to explaining in a legend (I'm thinking of background shading, and, to a lesser extent, ticks & sites on the X axis); also this is only relevant for a single tree, so the legend stuff in trees vs tree sequences could get messy. But other views welcome. |
If you're open to some bike-shedding @hyanwong, I'd like to suggest that it would be better to have slightly more contrast between red/pink than HotPink provides. |
After discussion with @jeromekelleher, I'm temporarily switching this to a draft PR again so that we can generalise the "edges above tree nodes" function in a separate PR first. |
How about PaleVioletRed @grahamgower? I think LightPink may be too faded to be easily visible: |
That's better, but the difference is still fairly small. I understand that red/green colourblindness is often driven by poor ability to distinguish reds from gray, so if you have any red-challenged friends, it might be worth asking them. |
I just checked with two colour-blind folks, they can see the difference in the PaleVioletRed example clearly |
That's really helpful. Thanks |
2dc4fac
to
6d945bc
Compare
OK, this now uses the new |
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, minor comments.
484f353
to
fdb46b8
Compare
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, thanks.
f524cc9
to
da53b90
Compare
Ready to merge I think - @benjeffery? |
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.
LGTM, one minor thing in the changelog.
da53b90
to
d5a0004
Compare
d5a0004
to
5b384a5
Compare
Description
New parameter
all_edge_mutations
(defaultFalse
, and only forTree.draw_svg()
notTreeSequence.draw_svg()
) that plots "extra" mutations on a single tree. The default is to plot these in HotPink, and to extend the X axis (if show, which it isn't by default), to show the position of the extra mutations outside the tree interval:Probably needs a few more tests, but opening this PR for feedback. I generalised the "background shading" function so that it could be used on a tree as well as a tree sequence.
Fixes #1253
PR Checklist: