Conversation
… tables to zarr v2
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1170 +/- ##
==========================================
- Coverage 92.44% 92.42% -0.02%
==========================================
Files 51 52 +1
Lines 7820 7857 +37
==========================================
+ Hits 7229 7262 +33
- Misses 591 595 +4
🚀 New features to boost your workflow:
|
| assert element_format in TablesFormats.values(), FormatVersionUnknownError( | ||
| element_type="table", version_encountered=element_format | ||
| ) |
There was a problem hiding this comment.
This assertion gets silenced when running code with python -O. An if raise is preferable. Please change.
There was a problem hiding this comment.
also here there is no raise, so even in the assertion is evaluated we would get an AssertionError, not FormatVersioUnknownError
| # Write the table to the path of the table group | ||
| table.write_zarr(store=resolved_store, consolidate_metadata=False) | ||
|
|
||
| table_group = group[name] |
There was a problem hiding this comment.
do we need to redefine table_group? If yes please explain in a comment why. If not please remove.
|
|
||
| table_group = group[name] | ||
| else: | ||
| table.strings_to_categoricals() |
There was a problem hiding this comment.
Here there is a problem: table.strings_to_categoricals() changes the dtype of the string column to a categorical. An in-place operation from a write operation is an unexpected side effect.
We need to find a different solution. I suggest to add an extra assert to test_sdata_with_nan_in_obs() to verify that the dtype of the obs columns of the input object does not change after write, and to run this against all the combinations of pandas and anndata via the hatch config you created.
| "prek>=0.4.11", | ||
| "ruff>=0.16.0", |
There was a problem hiding this comment.
Not something to do now, but after moving to the scverse cookiecutter these dev packages may change. We'll figure this out later.
| {value="anndata>=0.12,<0.13", if = ["0.12-2"]}, | ||
| {value="pandas>=2.3,<3", if = ["0.12-2"]}, |
There was a problem hiding this comment.
why isn't this included in the tests anndata>=0.12,<0.13 with pandas~=3.0? I would either include or explain why it's not in a comment.
There was a problem hiding this comment.
Also instead of the comment # every option where the if-condition is True gets included
I would consider adding these comments, to make the 3 cases immediate. But up to you.
# case 1
{value="anndata~=0.13", if = ["0.13-2"]},
{value="pandas>=2.3,<3", if = ["0.13-2"]},
# case 2
{value="anndata~=0.13", if = ["0.13-3"]},
{value="pandas~=3.0", if = ["0.13-3"]},
# case 3
{value="anndata>=0.12,<0.13", if = ["0.12-2"]},
{value="pandas>=2.3,<3", if = ["0.12-2"]},
# case 4: anndata xxx pandas yyy not added because zzz (or add missing case)
| element_type="table", version_encountered=element_format | ||
| ) | ||
|
|
||
| if element_format.zarr_format == 3 and version("anndata") >= "0.13": |
There was a problem hiding this comment.
The second boolean condition performs a string comparison that is evaluated lexicografically, not parsing the version numbers:
version("anndata") >= "0.2.0"
Out[5]: False
version("anndata")
Out[6]: '0.12.7'
There was a problem hiding this comment.
To fix you can wrap each string with packaging.version.Version()
|
One last thing, please rename the PR to a more descriptive text since upon release the automatically-generated release notes will use the PR title. |
2 participants
Issue:
#1152
Summary:
How to test:
.unsDataFrame become string"nan"after SpatialData Zarr round trip #1152 to cover all cases therehatch run test-anndata-pandas: test {args}. Check config before running, every script is run multiple times based on the matrix defined.Related PRs