feat(4.0): Add check for conditions that would cause anndata.write to fail. #643
Conversation
Bento007
requested review from
danieljhegeman,
nayib-jose-gloria,
jahilton and
corismall
Codecov Report
@@ Coverage Diff @@
## main #643 +/- ##
==========================================
+ Coverage 83.77% 83.81% +0.04%
==========================================
Files 19 19
Lines 1744 1749 +5
==========================================
+ Hits 1461 1466 +5
Misses 283 283
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
- Having mixed types in a column - Having none string values for Catagories
Bento007
changed the title
| catagory_types = {type(x) for x in column.dtype.categories.values} | ||
| if len(catagory_types) > 1 or str not in catagory_types: | ||
| self.errors.append( | ||
| f"Column '{column_name}' in dataframe '{df_name}' must only contain string catagories. Found {catagory_types}." |
There was a problem hiding this comment.
nit catagories -> categories
There was a problem hiding this comment.
same w/ catagory_types var name
There was a problem hiding this comment.
also, I think it is true that we only support str categories in our current schema definition. But are we 100% certain that will always be true? If this is an anndata constraint, mind linking me to a doc or comment specifying this?
There was a problem hiding this comment.
This is not documented in anndata because it is a bug. Anndata should support not string values as categories and this is fixed in anndata 0.10.0.
There was a problem hiding this comment.
Gotcha. Mind adding a comment to this effect--maybe a TODO so we remember to remove this check for anndata 0.10.0 and/or if we get a non-string categorical field in schema 5 we remember to highlight this anndata 0.8.0 bug to product?
| def schema_validate(h5ad_file, add_labels_file, ignore_labels, verbose): | ||
| @click.option( | ||
| "-w", | ||
| "--write-check", |
There was a problem hiding this comment.
@jahilton I'd like to get your thoughts on this new flag before I merge.
Reason for Change
Changes
Testing