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

[python] Add export for MultiscaleImage to SpatialData #3355

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

jp-dark
Copy link
Collaborator

@jp-dark jp-dark commented Nov 20, 2024

Add function to export a full MultiscaleImage to a SpatialData Image2DModel or Image3DModel.

Note: At the time of this PR, SpatialData is in the process of changing the multiscale image model to move from the legacy xarray_datatree.datatree.DataTree class to the new xarray.DataTree model. This PR implements both versions and checks which to use based on the SpatialData version.

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 68.96552% with 18 lines in your changes missing coverage. Please review.

Project coverage is 85.89%. Comparing base (c46175d) to head (44226aa).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3355      +/-   ##
==========================================
+ Coverage   85.66%   85.89%   +0.22%     
==========================================
  Files          57       56       -1     
  Lines        6210     6166      -44     
==========================================
- Hits         5320     5296      -24     
+ Misses        890      870      -20     
Flag Coverage Δ
python 85.89% <68.96%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 85.89% <68.96%> (+0.22%) ⬆️
libtiledbsoma ∅ <ø> (∅)
---- 🚨 Try these New Features:

@jp-dark
Copy link
Collaborator Author

jp-dark commented Nov 20, 2024

sc-57970

@jp-dark jp-dark force-pushed the dark/spatial-data-image-export/sc-57970 branch from ed063d2 to 522282a Compare November 21, 2024 20:37
@johnkerl johnkerl changed the title [python] Add export for MultiscaleImage to SpatialData [python] Add export for MultiscaleImage to SpatialData Nov 21, 2024
Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

🚢

@jp-dark jp-dark merged commit b84ea72 into main Nov 22, 2024
11 checks passed
@jp-dark jp-dark deleted the dark/spatial-data-image-export/sc-57970 branch November 22, 2024 16:40
Copy link
Collaborator

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

Hey, looks like this got merged while I was working on a review. I was about to give this code a shot, but will now do that from the main branch.

Still going to leave the other comments here as feedback

try:
import xarray as xr
except ImportError as err:
warnings.warn("Experimental spatial exporter requires the spatialdatda package.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
warnings.warn("Experimental spatial exporter requires the spatialdatda package.")
warnings.warn("Experimental spatial exporter requires the spatialdata package.")

import spatialdata as sd
from spatialdata.models.models import DataTree
except ImportError as err:
warnings.warn("Experimental spatial exporter requires the spatialdatda package.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
warnings.warn("Experimental spatial exporter requires the spatialdatda package.")
warnings.warn("Experimental spatial exporter requires the spatialdata package.")

patch = _str_to_int(split_version[2])
except ValueError as err:
raise ValueError(f"Unable to parse version {version}.") from err
print(f"Actual: {(major, minor, patch)} and Compare: {upper_bound}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(f"Actual: {(major, minor, patch)} and Compare: {upper_bound}")

Comment on lines 19 to 36
def _version_less_than(version: str, upper_bound: Tuple[int, int, int]) -> bool:
split_version = version.split(".")
try:
major = _str_to_int(split_version[0])
minor = _str_to_int(split_version[1])
patch = _str_to_int(split_version[2])
except ValueError as err:
raise ValueError(f"Unable to parse version {version}.") from err
print(f"Actual: {(major, minor, patch)} and Compare: {upper_bound}")
return (
major < upper_bound[0]
or (major == upper_bound[0] and minor < upper_bound[1])
or (
major == upper_bound[0]
and minor == upper_bound[1]
and patch < upper_bound[2]
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a heads up, tiledbsoma has packaging as a transient dependency. It comes with a robust version parser + even a version type: https://packaging.pypa.io/en/latest/version.html

It's mostly what we use for this kind of thing in scverse packages.

# of the DataTree.
if _version_less_than(sd.__version__, (0, 2, 5)):
return DataTree.from_dict(
{f"scale{index}": image for index, image in enumerate(image_data_arrays)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add the "scale" prefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

try:
import geopandas as gpd
except ImportError as err:
warnings.warn("Experimental spatial outgestor requires the geopandas package.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might make sense to import spatial data first, since geopandas is a hard dep there.

Then there isn't the UX of import, hit missing dep error, install geopandas, import, hit missing spatialdata

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.

3 participants