Put metadata and schemas in dict representations

[benjeffery]

Fixes #686
This is backward compatible with existing dict representations as schemas (and top-level metadata and schema) are not required on fromdict and if empty are not included in asdict.
Note that this also adds a metadata_schema argument to set_columns.

[codecov]

Codecov Report

Merging #687 into master will decrease coverage by 0.28%.
The diff coverage is 67.48%.

@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
- Coverage   87.71%   87.42%   -0.29%     
==========================================
  Files          23       23              
  Lines       17810    17963     +153     
  Branches     3526     3575      +49     
==========================================
+ Hits        15622    15705      +83     
- Misses       1073     1091      +18     
- Partials     1115     1167      +52     

Flag	Coverage Δ
#c_tests	88.96% <100.00%> (+0.01%)	⬆️
#python_c_tests	90.61% <67.48%> (-0.65%)	⬇️
#python_tests	99.00% <100.00%> (+<0.01%)	⬆️

Impacted Files	Coverage Δ
python/_tskitmodule.c	82.85% <63.69%> (-1.06%)	⬇️
python/tskit/tables.py	99.67% <100.00%> (+<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 99ebd93...d771988. Read the comment docs.

[benjeffery]

I'm missing some coverage, don't merge for now.

[benjeffery]

@jeromekelleher This is now fully-backward compatible with older asdict and fromdict. Should be good to go.

[jeromekelleher]

Looks good, thanks @benjeffery. I think there's a memory leak in the dict_encoding. Can you run stress_lowlevel and verify please? Otherwise, I think we need to get a little more test coverage on the behaviour when metadata_schema etc is missing from the dict encoding (not mapped to None, but not in the dictionary at all).

[jeromekelleher]

we should be able to just leave metadata_schema out of this, right?

[benjeffery]

We can't as this is calling the low-level method.

[jeromekelleher]

Should be OK now, right?

[benjeffery]

Yep gone in 682b97d

[benjeffery]

@jeromekelleher Thanks, think I have addressed them all. Sorry there were quite a few!

[jeromekelleher]

Some comments above @benjeffery. I think it's probably worth manually pasting in a few small examples of dict encoded output from msprime and tsinfer, just to make sure that we can actually parse these successfully.

As I was going through this, I realised that the dict encoding should really be versioned. Maybe we should add a key "encoding_version" = (major,minor) to the dictionary, which (if not present) defaults to 1.0. That way we can think a bit more clearly about how this encoding changes over time.

(Obviously need to be careful that this doesn't break old versions of tskit, though. Should be OK, shouldn't it?)

[jeromekelleher]

Should be OK now, right?

[benjeffery]

I've added a version in 37f2bd6. I've used 1.1 as 1.0 is the verison that had no version. In your comment you mention setting a default to 1.0 - I think this would go in code that is reading back the version, which we're not doing yet as we're not using it.

[benjeffery]

I've added a hardcoded msprime example - but I can't see where in tsinfer it is making dicts. A search for "asdict" fails in the tsinfer source.

[jeromekelleher]

Looks good, few small changes but ready for final review then I think?

[benjeffery]

@jeromekelleher Ok, let me know if good for a squash.

[jeromekelleher]

I'll give one more look over once #687 is merged @benjeffery, but I'd be surprised if anything else shows up at this point.

[benjeffery]

#687 is this PR?

[jeromekelleher]

#687 is this PR?

🤦

[jeromekelleher]

LGTM, squash'n'merge!