Skip to content
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

Update tskit #89

Merged
merged 2 commits into from
Sep 2, 2020
Merged

Update tskit #89

merged 2 commits into from
Sep 2, 2020

Conversation

petrelharp
Copy link
Contributor

@petrelharp petrelharp commented Aug 11, 2020

This goes along with MesserLab/SLiM#101 and supercedes #82.

This was referenced Aug 12, 2020
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with the code here, but the metadata schema stuff looks good - just a few comments.
Great to see the API being used. Sorry that you had to deal with some rough edges, hopefully mostly smoothed off now!

"is_null": {
"type": "integer",
"description": "Whether this node describes a 'null' (non-existant) chromosome.",
"binaryFormat": "B",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this isn't a ? (bool). The example below suggests it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because in the SLiM code they're defined as uint8_t. Maybe this would all work out fine and be equivalent? I don't know?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, they are boolean, yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think bool size is compiler-dependant in C++ so SLiM needs to be specify uint8_t. In python ? will always be one-byte. Should all work out fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - so I'll leave it as-is (right?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @benjeffery was saying we need to use ? in the schema, and make sure that the corresponding value in SLiM is always a uint8_t; we should never used b or B in the schema, and we should never use bool in the corresponding C++ code. Do I have that right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ? In python uint_8 in C++. Sorry I wasn't clearer!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I got it. =)

Copy link
Contributor Author

@petrelharp petrelharp Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 7534d9b

dump reference sequence; closes #84

mut time

test slim is using metadata

test file version

test versus old metadata decoding

benchmark script

readme note

.
@petrelharp petrelharp mentioned this pull request Aug 28, 2020
@petrelharp petrelharp merged commit cc8a234 into master Sep 2, 2020
@petrelharp petrelharp deleted the update_tskit branch September 6, 2020 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants