Skip to content
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

Added a spatialdata-io mirror #778

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

quentinblampey
Copy link
Contributor

Simple mirror so that spatialdata.io.xenium will call spatialdata_io.xenium if spatialdata-io is installed.

If not installed, it raises an error only when calling spatialdata.io.xenium, while spatialdata.io itself will not raise anything.

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.73%. Comparing base (30f8103) to head (aa6f40e).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/spatialdata/io/__init__.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   91.78%   91.73%   -0.06%     
==========================================
  Files          45       46       +1     
  Lines        7124     7128       +4     
==========================================
  Hits         6539     6539              
- Misses        585      589       +4     
Files with missing lines Coverage Δ
src/spatialdata/io/__init__.py 0.00% <0.00%> (ø)

@LucaMarconato LucaMarconato linked an issue Jan 5, 2025 that may be closed by this pull request
@LucaMarconato
Copy link
Member

LucaMarconato commented Jan 6, 2025

Thanks for the PR! I reviewed the code and tried and unfortunately realized that there are two problems.

The first is that while the following works:

python -c "import_spatialdata_io; import spatialdata; print(spatialdata.io.xenium)"     

This (in the same environment) doesn't:

python -c "import spatialdata; print(spatialdata.io.xenium)"     

This, not because spatialdata_io is not available, but because this is raised:

ImportError: cannot import name 'SpatialData' from partially initialized module 'spatialdata' (most likely due to a circular import) (/Users/macbook/embl/projects/basel/spatialdata/src/spatialdata/__init__.py)

We could maybe find a way around this, but there is actually another problem. I realized that users may be confused because they would be able to do spatialdata.io.xenium, but not from spatialdata.io import xenium.

I think we have to change a bit the syntax. We could aim at support from spatialdata.io import xenium (I think in this case we will have to add io as an explicit module, but as long as we don't add the name of the technologies it's still fine).

The other option woudl be to support sdata.io.xenium (sdata is an object); now accessors would work (but I don't like this other approach much because we want to use spatialdata_io to construct sdata).

@LucaMarconato
Copy link
Member

I implemented the approach based on the io module. I tested it successfully when spatialdata is installed but not spatialdata_io, and with the following commands:

python -c "from spatialdata.io import xenium; print(xenium)"  
python -c "import spatialdata.io; print(spatialdata.io.xenium)"

Also the IDE (PyCharm) seems to behave nicely with respect to docstring/autosuggest.

I am nevertheless less convinced about this approach because without an accessor we have less polished code (we now have the _io package and the io packages, and if we add more convenience bridges, we need to explicitly add more subpackages). Also there is not a big practical different to the users for now (they need to import spatialdata.io vs spatialdata_io).

If we repeat the approach with other packages (spatialdata_plot and napari_spatialdata), the advantages would be larger because everything is "contained" in a single package. On the other hand we probably want a single plotting entry-point at the object level (and not at the package level) that will dispatch to the proper backend, something like

sdata.plot(plotting_library='spatialdata-plot')

as opposed to

import spatialdata.spatialdata_plot
spatialdata.spatialdata_plot

Anyway, I would not overthink it. I marked this as experimental in the docstring; I'd merge and test it in action.

@LucaMarconato LucaMarconato merged commit bafdc1c into scverse:main Jan 6, 2025
8 checks passed
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.

Access spatialdata-io from spatialdata
2 participants