scverse / spatialdata

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

Made _locate_spatial_element public, renamed to locate_element() #427

Closed LucaMarconato closed 4 months ago

LucaMarconato commented 5 months ago

Closes #423.

codecov[bot] commented 4 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (98c5cbc) 91.68% compared to head (3d50252) 91.73%.

:exclamation: Current head 3d50252 differs from pull request most recent head 615bd79. Consider uploading reports for the commit 615bd79 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #427 +/- ## ========================================== + Coverage 91.68% 91.73% +0.05% ========================================== Files 37 37 Lines 5053 5048 -5 ========================================== - Hits 4633 4631 -2 + Misses 420 417 -3 ``` | [Files](https://app.codecov.io/gh/scverse/spatialdata/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse) | Coverage Δ | | |---|---|---| | [src/spatialdata/\_core/spatialdata.py](https://app.codecov.io/gh/scverse/spatialdata/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse#diff-c3JjL3NwYXRpYWxkYXRhL19jb3JlL3NwYXRpYWxkYXRhLnB5) | `93.39% <85.71%> (+0.49%)` | :arrow_up: | | [src/spatialdata/transformations/operations.py](https://app.codecov.io/gh/scverse/spatialdata/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scverse#diff-c3JjL3NwYXRpYWxkYXRhL3RyYW5zZm9ybWF0aW9ucy9vcGVyYXRpb25zLnB5) | `92.52% <25.00%> (ø)` | |
aeisenbarth commented 4 months ago

is there something specific that would prevent you from doing something like this next(key for key, value in dd.items() if value == 'value') on the various elements?

Not really, except that on first try I did exactly that and got a deep comparison of every nested xarray attribute and array chunk, which was a major performance hit. Now knowing SpatialData's private implementation, I can do:

next(name for name, image in sdata.images.items() if id(image) == id(element))

which I then would refactor out to a utils function, which is so generic (not specific to my module) that one would wonder whether it better lives in SpatialData itself. Especially since other users would implement the same utils function, possibly doing the same mistakes.

But using that code is fine for me!

giovp commented 4 months ago

thanks @aeisenbarth

which I then would refactor out to a utils function, which is so generic (not specific to my module) that one would wonder whether it better lives in SpatialData itself. Especially since other users would implement the same utils function, possibly doing the same mistakes.

this is a good enough use case. I would then add the method to both Elements but also SpatialData, wdyt @LucaMarconato ?

LucaMarconato commented 4 months ago

Thanks for the comments, sounds good to me.

@aeisenbarth I would return paths; I suggest to get the name of the path for the lookup until we implement lookup by path. To get the type of the element I recommend to use get_model().

LucaMarconato commented 4 months ago

@giovp ready for re-review (please see also my comment in the remaining unresolved conversation).

LucaMarconato commented 4 months ago

I have addressed all the comments (changed the return type to an optional list of strings). So ready to merge now.