scverse / spatialdata

An open and interoperable data framework for spatial omics data
https://spatialdata.scverse.org/
BSD 3-Clause "New" or "Revised" License
237 stars 45 forks source link

Move c_coords out of kwargs #779

Closed melonora closed 1 week ago

melonora commented 1 week ago

This PR implements one of the tasks in #750 to improve the handling of channel names. It moves the c_coords parameter out of kwargs and also adds a validation for a mismatch in length between c_coords and the length of the c dimension.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.84%. Comparing base (42f7b6a) to head (088dec2). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/spatialdata/models/models.py 88.88% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #779 +/- ## ========================================== - Coverage 91.85% 91.84% -0.01% ========================================== Files 45 45 Lines 6885 6893 +8 ========================================== + Hits 6324 6331 +7 - Misses 561 562 +1 ``` | [Files with missing lines](https://app.codecov.io/gh/scverse/spatialdata/pull/779?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse) | Coverage Δ | | |---|---|---| | [src/spatialdata/datasets.py](https://app.codecov.io/gh/scverse/spatialdata/pull/779?src=pr&el=tree&filepath=src%2Fspatialdata%2Fdatasets.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse#diff-c3JjL3NwYXRpYWxkYXRhL2RhdGFzZXRzLnB5) | `100.00% <ø> (ø)` | | | [src/spatialdata/models/models.py](https://app.codecov.io/gh/scverse/spatialdata/pull/779?src=pr&el=tree&filepath=src%2Fspatialdata%2Fmodels%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse#diff-c3JjL3NwYXRpYWxkYXRhL21vZGVscy9tb2RlbHMucHk=) | `87.76% <88.88%> (-0.01%)` | :arrow_down: |
melonora commented 1 week ago

still need to see how we can have c_coords only in docstring when dealing with images and not labels.

LucaMarconato commented 1 week ago

still need to see how we can have c_coords only in docstring when dealing with images and not labels.

Does this work now?

Btw, looks great, pre-approving. Only missing thing, please add some tests (testing that it works for images and testing that it gives an error for labels, and for images when there is a mismatch). Then feel free to merge, thanks!

melonora commented 1 week ago

Main problem is that the docstring must be a literal so you can't use f strings or anything.