Skip to content

AnnData obs/var string to categoricals - #1170

Open
ajkswamy wants to merge 16 commits into
mainfrom
iss1152
Open

AnnData obs/var string to categoricals#1170
ajkswamy wants to merge 16 commits into
mainfrom
iss1152

Conversation

@ajkswamy

Copy link
Copy Markdown
Collaborator

Issue:

#1152

Summary:

  • Writing AnnData table to zarr file now uses different methods depending on zarr format being written and anndata version installed
  • A new custom Deprecation warning is thrown when writing any element to zarr format v2.
  • A new hatch env config to help run all tests for combination of anndata/pandas versions

How to test:

Related PRs

@ajkswamy ajkswamy self-assigned this Jul 31, 2026
@ajkswamy
ajkswamy marked this pull request as ready for review July 31, 2026 16:12
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.40%. Comparing base (eb4fb3d) to head (08b811c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/spatialdata/_io/exceptions.py 60.00% 4 Missing ⚠️
src/spatialdata/_io/io_table.py 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
- Coverage   92.44%   92.40%   -0.04%     
==========================================
  Files          51       52       +1     
  Lines        7820     7858      +38     
==========================================
+ Hits         7229     7261      +32     
- Misses        591      597       +6     
Files with missing lines Coverage Δ
src/spatialdata/_core/spatialdata.py 93.86% <ø> (ø)
src/spatialdata/_io/io_points.py 100.00% <100.00%> (ø)
src/spatialdata/_io/io_raster.py 89.35% <100.00%> (+0.30%) ⬆️
src/spatialdata/_io/io_shapes.py 96.34% <100.00%> (+0.18%) ⬆️
src/spatialdata/_io/io_table.py 89.28% <90.47%> (-1.20%) ⬇️
src/spatialdata/_io/exceptions.py 60.00% <60.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ajkswamy
ajkswamy requested a review from ilan-gold August 3, 2026 06:44
Comment thread src/spatialdata/_io/io_table.py Outdated
Comment on lines +77 to +79
assert element_format in TablesFormats.values(), FormatVersionUnknownError(
element_type="table", version_encountered=element_format
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion gets silenced when running code with python -O. An if raise is preferable. Please change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here there is no raise, so even in the assertion is evaluated we would get an AssertionError, not FormatVersioUnknownError

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, done

Comment thread src/spatialdata/_io/io_table.py Outdated
# Write the table to the path of the table group
table.write_zarr(store=resolved_store, consolidate_metadata=False)

table_group = group[name]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to redefine table_group? If yes please explain in a comment why. If not please remove.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I have removed them

Comment thread src/spatialdata/_io/io_table.py Outdated

table_group = group[name]
else:
table.strings_to_categoricals()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right, I added the extra assert statements to test_sdata_with_nan_in_obs() and indeed there is a side effect caused by strings_to_categoricals().

anndata.AnnData.write_zarr has an argument convert_strings_to_categorical which is set to True by default. Hence the default round trip for nullable stings columns works by default with anndata, but with this side effect. Here, I have added an argument to io_table.write_table along with all functions in the call chain from Spatialdata.write that allows users the same options to convert strings in AnnData objects to categorical before writing. The main difference that we set the default value to be false, i.e., we do not convert by default and users need to explicitly set this to be able to do round trip with nullable string/object columns with pandas 2.0.

Comment thread pyproject.toml Outdated
Comment on lines +67 to +68
"prek>=0.4.11",
"ruff>=0.16.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something to do now, but after moving to the scverse cookiecutter these dev packages may change. We'll figure this out later.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread pyproject.toml Outdated
Comment on lines +270 to +271
{value="anndata>=0.12,<0.13", if = ["0.12-2"]},
{value="pandas>=2.3,<3", if = ["0.12-2"]},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LucaMarconato LucaMarconato Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment why the case of anndata>=0.12,<0.13 with pandas~=3.0 .

I have simplified the conditionals defining the dependency versions. For now, I prefer to keep them organized per dependency and version. We can modify them if needed later.

Comment thread src/spatialdata/_io/io_table.py Outdated
element_type="table", version_encountered=element_format
)

if element_format.zarr_format == 3 and version("anndata") >= "0.13":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix you can wrap each string with packaging.version.Version()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, done

@LucaMarconato

Copy link
Copy Markdown
Member

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.

@ajkswamy ajkswamy changed the title Iss1152 AnnData obs/var string to categoricals Aug 10, 2026
@ajkswamy
ajkswamy requested a review from LucaMarconato August 12, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants