Add downloadable cells dataset via scverse-misc - #1149
Conversation
Expose spatialdata.datasets.cells() alongside blobs/raccoon. It downloads the cells example dataset and loads it as a SpatialData object, reusing the scverse-misc datasets infrastructure (parse_registry + fetch with the built-in spatialdata loader) rather than reimplementing a downloader. - ship src/spatialdata/datasets.yaml registry (base_url + cells.zip sha256) - add scverse-misc[datasets]>=0.0.10 dependency - bump requires-python and ruff target to 3.12 (scverse-misc requires >=3.12) - update CI matrix 3.11 -> 3.12 - docs + network-free registry test and a slow download test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
7d60be6 to
c453909
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
==========================================
+ Coverage 92.40% 92.41% +0.01%
==========================================
Files 52 52
Lines 7858 7874 +16
==========================================
+ Hits 7261 7277 +16
Misses 597 597
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Didn't dive deep but this looks great. One important thing is missing, for each dataset we must list the license, and if the license requires attribution, we need to list that. Could you please add this? |
The cells dataset is derived from the 10x Genomics Xenium Prime Cervical Cancer FFPE sample, released under CC BY 4.0 (attribution required). Record the license and attribution in the registry and document the expectation that every dataset lists its license.
|
@LucaMarconato added the license info 👌 |
ajkswamy
left a comment
There was a problem hiding this comment.
LGTM, here are some suggestions for improvements:
- Please add a link to the associated PR in
spatialdata-notebooks. Please also add a couple of lines describing the motivation for this PR along which what value it adds. I see the assocated PR inspatialdata-notebooksalready has a couple of good lines in this direction.
| import pooch | ||
| from scverse_misc.datasets import fetch, parse_registry | ||
|
|
||
| cache_dir = Path(path) if path is not None else Path(pooch.os_cache("spatialdata")) |
There was a problem hiding this comment.
please also cover the "else" case here in tests.
| from scverse_misc.datasets import fetch, parse_registry | ||
|
|
||
| cache_dir = Path(path) if path is not None else Path(pooch.os_cache("spatialdata")) | ||
| registry = importlib.resources.files("spatialdata").joinpath("datasets.yaml") |
There was a problem hiding this comment.
I see the three lines here are also used in the unit test for this function. Would it make sense to make this into a separate function. Maybe it will be reused if we add more datasets that use this pattern from spatialdata-misc
Extract _shipped_registry() and _cache_dir() helpers, shared between cells() and the tests, removing the duplicated registry-parsing block. _cache_dir() isolates the path-vs-default branch so both sides are covered without a network call. Describe the dataset contents in the docstring, and assert the concrete element counts and shapes in test_cells_download. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
parse_registry is untyped in the mypy env, so returning its result directly tripped no-any-return; assign through annotated locals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| @pytest.mark.slow | ||
| def test_cells_download(tmp_path) -> None: | ||
| # Downloads ~3 MB from the scverse example data bucket; opt out with `-m "not slow"`. |
There was a problem hiding this comment.
Rather than opt-out, I'll try turning this into opt-in and skip the test by default.
There was a problem hiding this comment.
Done, made opt-in and opting in in the GitHub CI.
It's a small dataset here and not a big deal, but later when we test cloud support it's good that we differentiate between tests require a network and tests that do not.
Rename the `slow` marker to `network` and flip it from opt-out (`-m "not slow"`) to opt-in: tests marked `network` are skipped unless `--run-network` is passed. Enable the flag in CI so `test_cells_download` runs there, and document the `license_url` convention for CC-BY-style datasets in datasets.yaml.
Summary
Exposes
spatialdata.datasets.cells()alongsideblobs/raccoon. It downloads thecellsexample dataset and loads it as aSpatialDataobject.Dataset fetching is delegated to
scverse-misc[datasets](>=0.1.0, the first released version with the datasets feature). The registry shipped insrc/spatialdata/datasets.yamlis parsed and fetched (downloaded, hash-verified, cached, loaded) at call time.The actual download is exercised by
test_cells_download, markedslow; the network-freetest_cells_registryvalidates the shipped registry on every run.