Metadata docs #741

benjeffery · 2020-07-31T23:45:27Z

benjeffery · 2020-08-13T12:16:06Z

Ok, I've treid to address all the comments and have done a read through. Ready for a check over @jeromekelleher and @petrelharp!

codecov · 2020-08-13T12:38:21Z

Codecov Report

Merging #741 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #741   +/-   ##
=======================================
  Coverage   87.72%   87.72%           
=======================================
  Files          24       24           
  Lines       19394    19400    +6     
  Branches     3640     3640           
=======================================
+ Hits        17013    17019    +6     
  Misses       1290     1290           
  Partials     1091     1091

Flag	Coverage Δ
#c_tests	`85.33% <ø> (ø)`
#python_c_tests	`90.17% <100.00%> (+<0.01%)`	⬆️
#python_tests	`99.02% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/tskit/metadata.py	`98.47% <100.00%> (+0.03%)`	⬆️

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 2b12450...5240745. Read the comment docs.

jeromekelleher

Looks good, some minor comments.

docs/tutorial.rst

python/tskit/metadata.py

benjeffery · 2020-08-13T16:18:52Z

@jeromekelleher Fixed up. I think we wait for @petrelharp or @molpopgen to have a look before merging.

petrelharp · 2020-08-14T05:02:43Z

docs/metadata.rst

+which created the tree-sequence file, as the exact metadata
+stored will vary depending on the use case. Subsequent processes can add or modify the schemas
+if they wish to add or modify to the types (or encoding) of the metadata. Most users of tree-sequence
+files will not need to modify the schemas.


Perhaps add "The purpose of the schema is to say what information is stored in each metadata record, and how it is stored." - maybe after the first sentence?

Added in 837e0e8

petrelharp · 2020-08-14T05:05:00Z

docs/metadata.rst

+This codec places extra restrictions on the schema:
+
+#. Each property must have a ``binaryFormat``
+    This sets the binary encoding for used for the property.


"for used for"

Fixed in 0b31382

petrelharp

Looks good!

Something that I didn't see addressed is: what if the metadata don't match the schema? The tree sequence will be loaded (since metadata are decoded lazily), but you'll get errors when trying to decode it; and to figure out what's going on (and maybe retrieve the metadata) you might need to set the schema to the null schema and look at the raw bytes.

Oh, and also: should it be said somewhere that the metadata schema is stored as a string in the underlying tables? And that if created in python, this can be viewed by str(schema)?

petrelharp · 2020-08-14T05:15:36Z

docs/metadata.rst

+To determine the binary encoding of each property in the metadata the ``binaryFormat`` key is used.
+This describes the encoding for each property using ``struct``
+`format characters <https://docs.python.org/3/library/struct.html#format-characters>`_.
+For example an unsigned 8-byte integer can be specified with::


We should probably acknowledge some of the below as being modified from the struct docs?

Fixed in 3d63b49

petrelharp · 2020-08-14T05:17:42Z

docs/metadata.rst

+The codec stores the length of the array before the array data. The format used for the
+length of the array can be chosen with ``arrayLengthFormat`` which must be one
+of ``B``, ``H``, ``I``, ``L`` or ``Q`` which have the same meaning as in the numeric
+types above. ``Q`` is the default. As an example::


Fixed in fecbe45 with added error message on overflow.

petrelharp · 2020-08-14T05:21:21Z

docs/metadata.rst

+            "codec": "struct",
+            "type": "object",
+            "properties": {
+                "accession_number": {"type": "number", "binaryFormat": "i"},


shouldn't this be integer?

Fixed in 8b80596

petrelharp · 2020-08-14T05:23:34Z

docs/metadata.rst

+
+This schema states that the metadata for each row of the table
 is an object consisting of two properties. Property ``accession_number`` is a number
 (stored as a 4-byte int) which must be specified (it is included in the ``required``


they're both required, since it's struct, and there isn't a required list any more (should there be, to emphasize?)

Fixed in 4dfd481

petrelharp · 2020-08-14T05:27:58Z

docs/tutorial.rst

-
-    'Bob1234'
+To modify the metadata of rows in tables use the :ref:`sec_tutorial_metadata_bulk`.
+to


missing end of the sentence?

Fixed in 004263a

petrelharp · 2020-08-14T05:30:30Z

docs/tutorial.rst

+accessed with ``ts.metadata`` and ``ts.metadata_schema``.
+
+If there is no schema for a table or top-level metadata, then no decoding is performed
+and ``bytes`` will be returned.


Is "no schema" the same as MetadataSchema(None)?

Fixed in 2f5f18e

docs/tutorial.rst

petrelharp · 2020-08-14T05:40:02Z

docs/tutorial.rst

    tables.individuals.metadata_schema = schema

-Now that the table has a schema calls to
+This will overwrite any existing schema. Now that the table has a schema calls to


Perhaps it should be mentioned here that assigning the schema does not validate all pre-existing metadata entries.

Note added in a84fefb

benjeffery · 2020-08-14T12:01:34Z

Something that I didn't see addressed is: what if the metadata don't match the schema? The tree sequence will be loaded (since metadata are decoded lazily), but you'll get errors when trying to decode it; and to figure out what's going on (and maybe retrieve the metadata) you might need to set the schema to the null schema and look at the raw bytes.

Yes you'll likely get some hairy message from the decoder, or worse silently corrupted or missing data. There's not much that can be done apart from encouraging use of validate_and_encode_row rather than just encode_row.

benjeffery · 2020-08-14T12:49:37Z

Thanks for the great comments @petrelharp I think I've addressed them all.

petrelharp

Looks good to me! Thanks!!

jeromekelleher · 2020-08-17T07:38:53Z

Looks like this is ready to go after a squash @benjeffery?

benjeffery · 2020-08-17T08:49:50Z

I've squashed to two commits: one docs, one code change. Thanks for everyone's input to the metadata API and docs!

benjeffery mentioned this pull request Aug 10, 2020

metadata documentation #762

Closed

benjeffery requested a review from jeromekelleher August 13, 2020 12:14

benjeffery requested a review from petrelharp August 13, 2020 12:16

jeromekelleher reviewed Aug 13, 2020

View reviewed changes

docs/tutorial.rst Outdated Show resolved Hide resolved

docs/tutorial.rst Outdated Show resolved Hide resolved

docs/tutorial.rst Show resolved Hide resolved

python/tskit/metadata.py Show resolved Hide resolved

benjeffery marked this pull request as ready for review August 13, 2020 16:18

jeromekelleher approved these changes Aug 13, 2020

View reviewed changes

petrelharp reviewed Aug 14, 2020

View reviewed changes

benjeffery mentioned this pull request Aug 14, 2020

default arrayLengthFormat: Q or L? #769

Closed

petrelharp approved these changes Aug 14, 2020

View reviewed changes

benjeffery and others added 2 commits August 17, 2020 09:43

Improve metadata docs

c312c54

Fix doc for array length and check for overflow

5240745

benjeffery force-pushed the metadata-docs branch from 0b31382 to 5240745 Compare August 17, 2020 08:46

benjeffery added the AUTOMERGE-REQUESTED label Aug 17, 2020

mergify bot merged commit 8dcfa21 into tskit-dev:master Aug 17, 2020

mergify bot removed the AUTOMERGE-REQUESTED label Aug 17, 2020

benjeffery deleted the metadata-docs branch August 18, 2020 10:47

Metadata docs #741

Metadata docs #741

Uh oh!

Conversation

benjeffery commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjeffery commented Aug 13, 2020

Uh oh!

codecov bot commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjeffery commented Aug 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrelharp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Aug 14, 2020

Uh oh!

benjeffery commented Aug 14, 2020

Uh oh!

petrelharp left a comment

Choose a reason for hiding this comment

Uh oh!

jeromekelleher commented Aug 17, 2020

Uh oh!

benjeffery commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

benjeffery commented Jul 31, 2020 •

edited

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading