-
Notifications
You must be signed in to change notification settings - Fork 79
Set mutation positions using mutation time if available #1230
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
|
This took a bit of reworking to include mutations above the root. But worth sorting as this will impact implementing a Y-axis scale, which we can now do by looking at and this for a log time scale: |
jeromekelleher
left a comment
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, although haven't delved into all the details. Getting full coverage in tests will be a faff.
bad2a19 to
342cb7b
Compare
Codecov Report
@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
+ Coverage 93.72% 93.74% +0.01%
==========================================
Files 26 26
Lines 21532 21583 +51
Branches 909 921 +12
==========================================
+ Hits 20181 20232 +51
- Misses 1312 1313 +1
+ Partials 39 38 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
What extra coverage would you like? E.g. should we check that a mutation high above a root is still within the tree box, and that the root branch is long enough? You can see this in the example SVGs, but it's not explicitly checked, which would be sensible, I guess. I'm not sure what else would be useful. Perhaps an example from the WF simulator for some multi-root trees, especially if we can make the JukeCantor routine place mutations at defined times? |
|
My bad, looks like you have 100% coverage with the examples. |
342cb7b to
402119c
Compare
OK, review comments done. Probably worth saving more extensive testing until we have y-scales, when it should be a bit more obvious where the mutations are, whether it's a log scale, etc. So maybe merge this if it's OK, and I'll get on with that. |
402119c to
2a14c14
Compare
a427b2a to
3becc79
Compare
|
Rolled into #1236 too, so we could either close this PR and review all the changes together over there, or merge this first and rebase. Either is fine by me. |
|
Looks like a fair bit has changed in #1236, so will probably be reviewing twice if we go through it here too? |
Yeah, closing. |


Description
The first step to putting a y-axis on SVG plots.
Fixes #840
PR Checklist: