-
Notifications
You must be signed in to change notification settings - Fork 7
Fix for time_units in tskit 0.4.1 #55
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
Conversation
a1a9b37
to
3f2677b
Compare
Thanks for the report @rwaples! We'll get a release out ASAP! |
3f2677b
to
71dc019
Compare
@jeromekelleher A quick fix for now, but I think tszip should be enumerating the properties it can cope with, rather than assuming the ones it doesn't recognise are arrays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - can we do something with the types of the values instead?
tszip/compression.py
Outdated
for name in columns: | ||
if name.endswith("metadata_schema"): | ||
if name.endswith("metadata_schema") or name == "time_units": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would if isinstance(column[name], str)
work here? Is the metadata
column below also effectively assuming it's a bytes
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit that does that on compress. On decompress we can't detect, although the best idea might be to write the type to the zarr metadata. Will try that later.
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 97.70% 97.74% +0.04%
==========================================
Files 6 6
Lines 305 311 +6
Branches 55 61 +6
==========================================
+ Hits 298 304 +6
Misses 5 5
Partials 2 2
Continue to review full report at Codecov.
|
61f512f
to
e631e0d
Compare
@jeromekelleher I've updated this to store type info in the zarr - this should mean that tszip copes with any additional string or bytes attributes on tables. |
What's the status here @benjeffery - I think we decided to keep the file format changes for another version, and just do the decode side by name? |
Yep, I think I'll open another PR with those changes, and leave these here for reference if we do do the file change. |
Closing in favour of #58 |
Sounds good - could we open an issue to track the proposed changes to the file format? I think these would be an improvement and more future-proof. |
Fixes #54