-
Notifications
You must be signed in to change notification settings - Fork 92
Iss1152 #1170
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
base: main
Are you sure you want to change the base?
Iss1152 #1170
Changes from all commits
1c1606c
2672c01
a88accb
26a928f
247cdc6
75dcba9
129bf9e
5f4afca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,8 @@ extra = [ | |
| [dependency-groups] | ||
| dev = [ | ||
| "bump2version", | ||
| "prek>=0.4.11", | ||
| "ruff>=0.16.0", | ||
| ] | ||
| test = [ | ||
| "pytest", | ||
|
|
@@ -244,3 +246,27 @@ memray-flame = "memray flamegraph --temporal" | |
|
|
||
| [tool.pixi.environments] | ||
| profiling = { features = ["profiling"], solve-group = "default" } | ||
|
|
||
| [tool.hatch.envs.test] | ||
| dependency-groups = ["test"] | ||
|
|
||
| [tool.hatch.envs.test-anndata-pandas] | ||
| template = "test" | ||
| extra-dependencies = ["zarr>=3"] | ||
| scripts.test = ["pip list|grep anndata && pip list|grep pandas && pytest {args}"] | ||
| scripts.test-readwrite = ["pip list|grep anndata && pip list|grep pandas && pytest tests/io/test_readwrite.py"] | ||
| scripts.test-all = ["pip list|grep anndata && pip list|grep pandas && pytest ."] | ||
|
|
||
| [[tool.hatch.envs.test-anndata-pandas.matrix]] | ||
| anndata-pandas = ["0.13-2", "0.13-3", "0.12-2"] | ||
|
|
||
| [tool.hatch.envs.test-anndata-pandas.overrides] | ||
| matrix.anndata-pandas.extra-dependencies = [ | ||
| # every option where the if-condition is True gets included | ||
| {value="anndata~=0.13", if = ["0.13-2"]}, | ||
| {value="pandas>=2.3,<3", if = ["0.13-2"]}, | ||
| {value="anndata~=0.13", if = ["0.13-3"]}, | ||
| {value="pandas~=3.0", if = ["0.13-3"]}, | ||
| {value="anndata>=0.12,<0.13", if = ["0.12-2"]}, | ||
| {value="pandas>=2.3,<3", if = ["0.12-2"]}, | ||
|
Comment on lines
+270
to
+271
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why isn't this included in the tests
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also instead of the comment I would consider adding these comments, to make the 3 cases immediate. But up to you. |
||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from ome_zarr.format import Format | ||
|
|
||
|
|
||
| class FormatVersionUnknownError(ValueError): | ||
| """Exception raised when an unknown element format is encountered.""" | ||
|
|
||
| def __init__(self, element_type: str, version_encountered: Format): | ||
| self.element_type = element_type | ||
| self.version_encountered = version_encountered | ||
| self.message = ( | ||
| f"Encountered unknown element format version " | ||
| f"`{self.version_encountered}` for element of type `{self.element_type}`" | ||
| ) | ||
| super().__init__(self.message) | ||
|
|
||
|
|
||
| class WritingToZarrV2DeprecationWarning(DeprecationWarning): | ||
| """Warning raised when writing to zarr v2 format.""" | ||
|
|
||
| message = ( | ||
| "Writing to zarr v2 format is currently deprecated in spatialdata " | ||
| "and will be removed in a future version. " | ||
| "Please consider writing to zarr v3." | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import warnings | ||
| from importlib.metadata import version | ||
| from pathlib import Path | ||
|
|
||
| import numpy as np | ||
|
|
@@ -9,6 +11,8 @@ | |
| from anndata._io.specs import write_elem as write_adata | ||
| from ome_zarr.format import Format | ||
|
|
||
| from spatialdata._io._utils import _resolve_zarr_store | ||
| from spatialdata._io.exceptions import FormatVersionUnknownError, WritingToZarrV2DeprecationWarning | ||
| from spatialdata._io.format import ( | ||
| CurrentTablesFormat, | ||
| TablesFormats, | ||
|
|
@@ -56,16 +60,43 @@ def write_table( | |
| group_type: str = "ngff:regions_table", | ||
| element_format: Format = CurrentTablesFormat(), | ||
| ) -> None: | ||
| if element_format.zarr_format == 2: | ||
| warnings.warn( | ||
| message=WritingToZarrV2DeprecationWarning.message, category=WritingToZarrV2DeprecationWarning, stacklevel=2 | ||
| ) | ||
|
|
||
| if TableModel.ATTRS_KEY in table.uns: | ||
| region, region_key, instance_key = get_table_keys(table) | ||
| TableModel.validate(table) | ||
| else: | ||
| region, region_key, instance_key = (None, None, None) | ||
|
|
||
| write_adata(group, name, table) | ||
| tables_group = group[name] | ||
| tables_group.attrs["spatialdata-encoding-type"] = group_type | ||
| tables_group.attrs["region"] = region | ||
| tables_group.attrs["region_key"] = region_key | ||
| tables_group.attrs["instance_key"] = instance_key | ||
| tables_group.attrs["version"] = element_format.spatialdata_format_version | ||
| # Ensure the table group exists | ||
| table_group = group.require_group(name=name) | ||
|
|
||
| assert element_format in TablesFormats.values(), FormatVersionUnknownError( | ||
| element_type="table", version_encountered=element_format | ||
| ) | ||
|
Comment on lines
+77
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion gets silenced when running code with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also here there is no |
||
|
|
||
| if element_format.zarr_format == 3 and version("anndata") >= "0.13": | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fix you can wrap each string with |
||
| # `write_zarr` in anndata v0.13 and above can only write to zarr v3 | ||
| # solution of passing resolved store directly roughly based on: | ||
| # https://github.com/scverse/anndata/issues/1548#issuecomment-2199801855 | ||
|
|
||
| # resolve the store from the group | ||
| resolved_store = _resolve_zarr_store(table_group) | ||
|
|
||
| # Write the table to the path of the table group | ||
| table.write_zarr(store=resolved_store, consolidate_metadata=False) | ||
|
|
||
| table_group = group[name] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to redefine |
||
| else: | ||
| table.strings_to_categoricals() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here there is a problem: We need to find a different solution. I suggest to add an extra assert to |
||
| write_adata(group, name, table) | ||
| table_group = group[name] | ||
|
|
||
| table_group.attrs["spatialdata-encoding-type"] = group_type | ||
| table_group.attrs["region"] = region | ||
| table_group.attrs["region_key"] = region_key | ||
| table_group.attrs["instance_key"] = instance_key | ||
| table_group.attrs["version"] = element_format.spatialdata_format_version | ||
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.
Not something to do now, but after moving to the scverse cookiecutter these dev packages may change. We'll figure this out later.