SpatialData allows an element to be contained more than once with different names. When checking whether it is contained, the result is unexpectedly False although the element is contained.
Example
fromspatialdataimportSpatialDatafromspatialdata.modelsimportImage2DModelimportnumpyasnpimg=Image2DModel.parse(np.arange(4).reshape((1, 2, 2)), dims=("c", "y", "x"))
sdata=SpatialData(images={"img1": img, "img2": img})
assertsdata.contains_element(img)Cause
_locate_spatial_element raises a ValueError if the element is not found and another ValueError if the element is found more than once. contains_element catches both and returns False, but it should catch the second case separately and return True.
It's maybe worth to consider whether to disallow the same element under different keys (how would that be represented on disk?), because it is a very special edge case that breaks many other code places of spatialdata (e.g. _write_transformations_to_disk) and would need to be completely covered by test cases (if it has already any).
Instead of having contains_element inspect the AssertionError string from locate_spatial_element to distinguish the two ValueErrors, we could either have ValueError subclasses, or a private helper method that returns the counts of found elements (0, 1, 2…) instead of raising exceptions.
SpatialData allows an element to be contained more than once with different names. When checking whether it is contained, the result is unexpectedly False although the element is contained.
Example
Cause
_locate_spatial_elementraises a ValueError if the element is not found and another ValueError if the element is found more than once.contains_elementcatches both and returns False, but it should catch the second case separately and return True.It's maybe worth to consider whether to disallow the same element under different keys (how would that be represented on disk?), because it is a very special edge case that breaks many other code places of spatialdata (e.g.
_write_transformations_to_disk) and would need to be completely covered by test cases (if it has already any).Instead of having
contains_elementinspect the AssertionError string fromlocate_spatial_elementto distinguish the two ValueErrors, we could either have ValueError subclasses, or a private helper method that returns the counts of found elements (0, 1, 2…) instead of raising exceptions.