Fix four behaviour changes introduced by the mypy adoption in #411 - #418
Merged
Conversation
`_get_reader()` auto-detection was changed in #411 from a real `import rioxarray` to `importlib.util.find_spec("rioxarray")`. `find_spec` only reports whether the module can be *found*; importing it can still fail, which is what a `rioxarray` with a broken `rasterio` looks like. Before, that raised `ModuleNotFoundError` and the reader silently used the `dask_image` backend. Now the rioxarray backend is selected, and its own internal import turns the failure into "Using rioxarray backend requires to install the rioxarray library", which misdescribes the cause and leaves the user with no working backend. Use `importlib.import_module()` so the import is actually attempted (a plain `import rioxarray` would be flagged as unused), and catch `ImportError` rather than only `ModuleNotFoundError`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#411 replaced if path is not None: path = Path(path) if not os.path.isdir(path): raise FileNotFoundError(...) with `path = Path() if path is None else Path(path)`, to drop a `# type: ignore` on the `_check_path(path=path)` calls. `Path()` is `.`, which is always a directory, so `dbit()` with no `path` now searches the process's current working directory and reads whatever `.h5ad` and barcode file happen to be there. Previously that combination raised (`Path.joinpath(None, ...)` -> `AttributeError`). Restore the original handling and type `_check_path`'s `path` as `Path | None`, raising there when a directory search is needed but no directory was given. `dbit(anndata_path=..., barcode_position=...)` without a `path` keeps working, as it did before #411. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ositions
Two problems, both of which abort `macsima()` on a folder containing one
truncated TIFF.
`create_sdata()`'s physical-size loop exists to tolerate invalid files
("Iterate over path files, as it may still contain invalid files"), but
#411 narrowed its `except Exception` to
`(OSError, ValueError, IndexError, NotImplementedError)`. The failure
modes of `ome_types.from_tiff()` are not enumerable: a TIFF with a valid
header truncated before its metadata raises `struct.error`, which is
none of those, and tifffile's own `TiffFileError` has changed base class
between releases.
`MultiChannelImage.from_paths()` has the same problem with
`except ValueError` and, being reached first, is what actually raises
today. That guard predates #411; fixing only one of the two leaves the
reader broken, so both are widened, with a comment explaining why a
blind except is correct here.
Separately, `_get_translations()` truncates the OME plane positions with
`int()`. They are used as `da.pad` widths so they do have to be
integers, but truncation biases every offset towards the origin
(`0.9 -> 0`); round instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plain Visium reader had no test, which is how a hard `TypeError` on the first call shipped in #411. Add the 10x Space Ranger 2.0.0 CytAssist FFPE Human Breast Cancer dataset (CC BY 4.0) to the test-data workflow and assert the property that broke: the circles are the spot coordinates of the table, in the order of the table, with the radius taken from the scalefactors. The expected coordinates are read from `spatial/tissue_positions.csv` independently of the reader. Only the filtered matrix and the `spatial/` archive are downloaded (~65 MB); the full-resolution tissue image is ~2 GB and `fullres_image_file` is optional. The test skips until the data artifact is regenerated by running the `Prepare test data` workflow, as its header describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LucaMarconato
force-pushed
the
fix/pr-411-regressions
branch
from
September 9, 2026 11:21
22489c4 to
f30c00f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
+ Coverage 63.13% 63.53% +0.39%
==========================================
Files 26 26
Lines 3258 3263 +5
==========================================
+ Hits 2057 2073 +16
+ Misses 1201 1190 -11
🚀 New features to boost your workflow:
|
`space-ranger/latest/resources/visium-example-data` does not exist: only Visium HD has an `*-example-data` page, and the Space Ranger docs point non-HD Visium users to the datasets portal. Cite the portal instead. The dataset URL in the skip message was also a guess, and its slug belongs to a different dataset (Space Ranger 1.3.0, not the CytAssist 2.0.0 one used here). Point at the download prefix the workflow actually uses, which is verifiable, and at the workflow itself for the layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The five "Fixed" entries described bugs introduced by the cookiecutter migration (#411), which landed after v0.7.1 and has never been released. Reporting them as fixes is misleading: no released version ever had them, so there is nothing for a user to have hit. They are internal churn within the template migration that the "Changed" entry already covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reshape the single regression test into a reader test suite in the style of the xenium, visium_hd and macsima ones: element names and coordinate systems, the extent of each coordinate system, the two downscaled images, the table and its annotation, the circles (the former regression test, kept as the check that the circles are the spot coordinates in the order of the table), the dataset_id inferred from the counts file, and a CLI roundtrip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whether the plane positions can be used as pixel padding widths at all is a question for the MACSima maintainers, not something to change in a regression fix: rounding them only removes the sub-pixel bias of `int()` and leaves the larger question of the unit open. Tracked in an issue instead. The widened `except` clauses stay: without them one unreadable file in the folder aborts the reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test only checked that the reader survives the truncated TIFF; it now also pins the warning that says which file was skipped, so a silent skip fails too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`mypy src tests` follows `tests/data`, one of the two gitignored locations the test datasets are downloaded to, and type-checks whatever scripts happen to sit next to the data. CI never sees it — it downloads the artifact to `data/` and the pre-commit job downloads nothing — but locally the hook fails on files that are not ours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LucaMarconato
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #417. All four problems were introduced by the
mypyadoption in #411, where a type error was silenced by restructuring code rather than by annotating it.AI-generated.
Changes
b3e50bfmerscopeimportlib.import_module("rioxarray")instead offind_spec, so arioxarraythat is installed but cannot be imported (brokenrasterio) falls back to thedask_imagebackend again. CatchesImportError, not onlyModuleNotFoundError6b908f5dbitpathhandling sodbit()no longer defaults toPath(".")and reads the cwd._check_path'spathis typedPath | Noneand raises when a directory search is needed but no directory was given, which is what the removed# type: ignorewas standing in for.dbit(anndata_path=..., barcode_position=...)keeps workingaba347dmacsimacreate_sdata's physical-size loop, narrowed in #411, andMultiChannelImage.from_paths, whoseexcept ValueErrorpredates it) — a truncated TIFF raisesstruct.error49c5d20,1243e53visiumf921772bd5ddfcmypyno longer followstests/datainto the downloaded datasetsProblem 1 of #417, the
visium()TypeError, is already fixed onmainby #416; this branch is rebased on top of it and adds the tests that were missing.The two
except Exceptionguards carry# noqa: BLE001and a comment: they apply a third-party parser to arbitrary files the user left in a folder, so the failure modes are not enumerable —struct.erroris one, andtifffile.TiffFileErrorhas changed base class between releases.Dropped since the first version of this PR
aba347doriginally also rounded the OME plane positions instead of truncating them withint(). That is reverted in7ff0ebe: the positions are physical coordinates (Plane.position_x, carrying aposition_x_unit) used directly asda.padwidths in pixels, so the unit may be the real problem, and rounding only removes a sub-pixel bias. Deciding that needs someone with real MACSima data, so it is #419 now, and this branch leaves the behaviour as it is onmain.No changelog entry
f921772removes the### Fixedblock rather than extending it.v0.7.1was tagged on 2026-05-15, #411 landed on 2026-08-11, and no tag contains it: none of these bugs ever reached a release, so describing them as fixes would announce breakage no user could have hit. The net diff againstmainis the removal of the entry #416 added; the regressions are churn inside the template migration that the existing### Changedentry already covers.Tests
visiumhad no test at all, which is how a hardTypeErroron the first call shipped. This adds the 10x Space Ranger 2.1.0 CytAssist FFPE Human Breast Cancer dataset (CC BY 4.0) toprepare_test_data.yaml— only the filtered matrix and thespatial/archive, ~65 MB; the full-resolution image is ~2 GB andfullres_image_fileis optional — and builds a reader test suite on it, in the style of thexenium,visium_hdandmacsimaones: element names and coordinate systems, the extent of each of the three coordinate systems, the two downscaled images, the table and its annotation, the circles, thedataset_idinferred from the counts file, and a CLI roundtrip.The property that broke is
test_visium_circles_are_the_spot_coordinates: the circles are the spot coordinates of the table, in the order of the table, with the radius from the scalefactors, compared againstspatial/tissue_positions.csvread independently of the reader.tests/test_visium.pyskips until someone regenerates the data artifact by running thePrepare test dataworkflow, as its header describes. It was verified locally against that dataset: withreaders/visium.pyatd2664fathe reader raises theTypeErrorof #417 and all nine fail (one failure, eight fixture errors); on this branch all nine pass.New tests, each verified to fail without its fix:
tests/test_visium.py— nine tests on real data, the regression above among them.tests/test_merscope.py—_get_readerwith arioxarrayonsys.paththat raises on import.tests/test_dbit.py—_check_pathwith no directory raises instead of searching the cwd; an explicitly given file still works.tests/test_macsima.py—macsima()on a copy ofOMAP10_smallwith one real tif truncated to 200 bytes, asserting both that the reader completes and that the warning names the file it skipped.Not addressed here
Listed in #417, left alone deliberately: the
xeniumcell_polys is not Nonedead branch, thecurioINSTANCE_KEY/index ordering,merscope's wrong_get_readerreturn annotation, theMIN_VERSION = (3, 13)gate that skips nine reader tests on the 3.12 CI leg, and four pre-existing bugs noticed nearby (curio's discardedvar.join, the missingbreakinmacsima's physical-size loop,codex's always-true format detection and its two missingpath /prefixes,dbit's write to a view).The two open questions on the
macsimapadding feature (#371) are now #419, and still want a maintainer with real data: whether the plane positions are µm rather than pixels, and that the whole translation path is uncovered because the OMAP test data hasposition_x = None.Verification
The four touched test files —
tests/test_visium.py,tests/test_merscope.py,tests/test_dbit.py,tests/test_macsima.py— are 74 passed locally, with the datasets on disk (Python 3.13,spatialdata0.8.1.dev,anndata0.13.3,pandas2.3.3,tifffile2025.12.20). The rest of the suite runs in CI.prek run --all-filespasses,mypy src testsincluded.