-
Notifications
You must be signed in to change notification settings - Fork 84
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
Change end time inequality #1521
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1521 +/- ##
=======================================
Coverage 91.37% 91.37%
=======================================
Files 23 23
Lines 10105 10105
Branches 2119 2119
=======================================
Hits 9233 9233
Misses 438 438
Partials 434 434
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I fiddled with tests a bit to get them to pass. There were a few testing the timing of some events when it equaled the set end time, of course, so would be good to check that I haven't made those tests useless. Also, two tests were failing in lib/tests/ancestry.c, and I'm wondering if lines 373 and 489 should be changed to |
Excellent, thanks @apragsdale! Yes, I think you're right we can change those C tests. This does mean that our semantics wrt events like sampling etc is slightly different than before, I guess it's unlikely anyone is actually depending on this. This is the sort of stuff we want to break for 1.0 anyway, while we're breaking stuff. We should add a note to the CHANGELOG to make sure we don't forget to tell people about it, though.l |
Ok, changelog updated. Let me know if you want any changes made here, or if I should rebase/etc |
Looks great, thanks @apragsdale. I think we just need to update the documentation for the |
Is this ok to merge? Not sure what to do about the one codecov failure here.. |
@Mergifyio please rebase |
Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
@Mergifyio rebase |
Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
@Mergifyio rebase |
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, thanks @apragsdale!
Command
|
0a074c1
to
1cf5a99
Compare
Thanks Jerome - glad to get this sorted! |
Thanks for sorting it out @apragsdale! |
Closes #1304
Some tests are still failing here.. trying to figure out what's going on