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
compute mutation parents on text input #2730
base: main
Are you sure you want to change the base?
Conversation
Closes #2729. |
@Mergifyio rebase |
Whoops; looks like I've made some silly mistake. |
Okay - I got this to work; I had to rearrange the tables in one of the tests. HOWEVER, I'm worried that putting in |
Codecov Report
@@ Coverage Diff @@
## main #2730 +/- ##
=======================================
Coverage 94.08% 94.08%
=======================================
Files 29 29
Lines 28516 28516
Branches 1609 1609
=======================================
Hits 26828 26828
Misses 1653 1653
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
I'm tending to agree actually. These functions are just for creating test examples, and it'll be easy to forget that any mutation parents you specify will be nuked by this call. Maybe we should either leave it as it is, or only compute mutation parents when that column is missing from the input? |
✅ Branch has been successfully rebased |
Maybe we add a flag to the function, that if true does indexes and computes mutation parents, and if false then does not? That'd make it real hard to miss the fact that the function does or doesn't do it. |
Over in #2729 we saw a bug due to not computing mutation parents on text input. This isn't really a bug, as the text input is not part of the public API, but I could easily imagine us making the same error in test code.
(More generally we ought to think about doing this as part of tree sequence instantiation; I seem to recall us having a reason not to do it, but I don't remember specifically what it was?)