You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR fixes#664 (I hope). Review requested from @petrelharp. Right now I've tested that it round-trips a tree sequence out to disk and back, and that seems to work AFAICT. Let me know if it does what you need it to do.
Also, note: it does what it does in an unorthodox way, by munging around inside the tskit data structures, because I didn't want to be making copies of this stuff – the derived state column could potentially be a pretty big buffer, so I didn't want to push the high-water memory usage mark higher if possible. If there is a cleaner and more orthodox way to do what I'm doing, that would be great of course. In any case, please make sure that the way that I'm munging tskit's data structures is completely correct and safe (given the current tskit implementation). This code ought to be bulletproof, of course, but I'm not sure that it is since I don't feel 100% confident in my understanding of how tskit does things under the hood (and AFAIK that is not documented anywhere, even in comments, right?).
And again GitHub Desktop has based this PR on master instead of multitrait. I definitely told the GUI to base it off of multitrait; I was careful! So, hmm, annoying. Can you fix it up again, however you do that, @petrelharp? (Is there UI in GitHub to do it, somewhere?) [EDIT: Found the UI, which is quite hidden! You have to click the "Edit title" button, and having done that, a popup appears that lets you choose the branch the PR will be merged into. Their UI designers were smoking something when they came up with that! :-O]
❌ Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.92%. Comparing base (3d2c471) to head (f31ec10).
Hmm. The branch that this PR is based on, derived_state_metadata, still appears to be based on master. So maybe this is still screwed up. I'm going to just delete this PR, and its associated branch, and try again. Grr.
OK, clearly I'm (yet again) fundamentally confused by Git's conceptual model. I just made a new branch, derived_state_metadata_2, based on multitrait. I published that branch. If I now look at it in GitHub, it says "This branch is 335 commits ahead of master." I was expecting it to say "This branch is 0 commits ahead of multitrait." Why is it comparing itself to master?? But if I go to Terminal and do git reflog show derived_state_metadata_2, it says "Created from multitrait", and it looks like HEAD for derived_state_metadata_2 points to the current head of multitrait. Aargh, so confusing. So I'm going to hypothesize that the original derived_state_metadata branch was OK, and that once I switched this PR over to being based against master (why didn't it base off of multitrait, since it knows that derived_state_metadata is a branch off of multitrait??), now this PR is also OK. So I'm going to reopen this PR, and delete my derived_state_metadata_2 branch. But it'd be helpful to understand why Git is acting this way, and to be reassured that merging this PR will not somehow corrupt the project. Grr, grr.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #664 (I hope). Review requested from @petrelharp. Right now I've tested that it round-trips a tree sequence out to disk and back, and that seems to work AFAICT. Let me know if it does what you need it to do.
Also, note: it does what it does in an unorthodox way, by munging around inside the tskit data structures, because I didn't want to be making copies of this stuff – the derived state column could potentially be a pretty big buffer, so I didn't want to push the high-water memory usage mark higher if possible. If there is a cleaner and more orthodox way to do what I'm doing, that would be great of course. In any case, please make sure that the way that I'm munging tskit's data structures is completely correct and safe (given the current tskit implementation). This code ought to be bulletproof, of course, but I'm not sure that it is since I don't feel 100% confident in my understanding of how tskit does things under the hood (and AFAIK that is not documented anywhere, even in comments, right?).