-
Notifications
You must be signed in to change notification settings - Fork 74
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
Implement TreeSequence.discrete_genome #1819
Conversation
dc5e308
to
b57b0e7
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.
LGTM! Just that one error on the CI. I guess we don't need to test the negative space as it isn't a valid tree sequence even though TableCollection allows it.
c/tskit/trees.c
Outdated
static inline bool | ||
is_discrete(double x) | ||
{ | ||
return floor(x) == x; |
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.
Do floating point representations of integers always resolve to the positive side? Did a quick google and didn't find a source.
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.
Right, good point. I've changed to trunc
just in case we ever end up supporting negative coords.
b57b0e7
to
3ba31d4
Compare
Codecov Report
@@ Coverage Diff @@
## main #1819 +/- ##
==========================================
- Coverage 93.90% 93.39% -0.51%
==========================================
Files 27 27
Lines 24634 24814 +180
Branches 1094 1094
==========================================
+ Hits 23132 23175 +43
- Misses 1467 1604 +137
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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, on a quick scan!
3ba31d4
to
90f62be
Compare
I've fixed the conflict and set it to merge |
Thanks @benjeffery - I didn't see the conflict, sorry. |
Closes #1144
This should unplug a few outstanding issues. We could make things a bit more granular and just focus on discrete sites/breakpoints etc, but I don't think it's worth the bother. Tree sequences mixing discrete and continuous coordinates are going to be rare, and probably a result of a mistake somewhere along the way.