Skip to content

Conversation

@benjeffery
Copy link
Member

@benjeffery benjeffery commented Mar 23, 2020

To split #491 up a bit I'm focusing on getting the C API right first.
Proof-of-concept for the python is done in #491 meaning I'm pretty sure this is the (per-table) changes we want. Only individuals is modified for now. Once everyone is happy I'll do the other tables.
I intend to do top-level (per tree sequence) metadata in another PR.

@benjeffery
Copy link
Member Author

@jeromekelleher You mentioned text dump round tripping - is that in python only? Couldn't see it in the C.

@jeromekelleher
Copy link
Member

@jeromekelleher You mentioned text dump round tripping - is that in python only? Couldn't see it in the C.

Yep. Text support in C is strictly just for testing. Life is too short to write production quality text parsing in C!

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM @benjeffery. Minor points above.

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #493 into master will decrease coverage by 0.11%.
The diff coverage is 73.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   86.96%   86.84%   -0.12%     
==========================================
  Files          21       21              
  Lines       15748    15944     +196     
  Branches     3066     3095      +29     
==========================================
+ Hits        13695    13847     +152     
- Misses       1034     1050      +16     
- Partials     1019     1047      +28     
Flag Coverage Δ
#c_tests 87.77% <73.55%> (-0.18%) ⬇️
#python_c_tests 90.52% <ø> (ø)
#python_tests 99.18% <ø> (ø)
Impacted Files Coverage Δ
c/tskit/tables.c 77.73% <73.55%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f53ff61...363b49d. Read the comment docs.

@jeromekelleher
Copy link
Member

LGTM, ready to copy-paste across many tables I think!

@benjeffery
Copy link
Member Author

@jeromekelleher I've given this another pass, after what we talked about with edge metadata. I've made metadata_schema an optional load and memcpy'd it out of kastore.
This still has metadata_schema == NULL for empty schemas. I think this makes sense for this, where it didn't for columns.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM @benjeffery, I'd say go for it. The NULL vs non-NULL thing is a bit philosophical when metadata_length == 0, so I think setting it to NULL on 0 is the right thing to do.

@benjeffery
Copy link
Member Author

Ok, will copy across to other tables now.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM @benjeffery, just spotted a minor typo in the docs. Shall we merge this much and follow up with the Python tests?

@benjeffery benjeffery changed the title Metadata C API only (WIP) [C API] Metadata schema field on tables Apr 1, 2020
@benjeffery
Copy link
Member Author

benjeffery commented Apr 1, 2020

@jeromekelleher Squashed rebased and ready to merge (pending CI!). Yes, lets merge here as this is a natural break point as nothing is broken, just added.

@benjeffery
Copy link
Member Author

C lint has failed as azure is on its knees. I don't have permission to re-run it.

@jeromekelleher jeromekelleher merged commit cbeb129 into tskit-dev:master Apr 1, 2020
@jeromekelleher
Copy link
Member

C lint has failed as azure is on its knees. I don't have permission to re-run it.

I've been getting these errors a bit elsewhere too - tried rerunning them but they kept failing.

@benjeffery
Copy link
Member Author

I'll see if we can remove the dependency

@benjeffery benjeffery deleted the metadata_c_only branch April 1, 2020 11:15
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.

2 participants