Repository navigation
contains_element returns False if element is contained more than once #425
Description
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
from spatialdata import SpatialData
from spatialdata.models import Image2DModel
import numpy as np
img = Image2DModel.parse(np.arange(4).reshape((1, 2, 2)), dims=("c", "y", "x"))
sdata = SpatialData(images={"img1": img, "img2": img})
assert sdata.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.
Activity
LucaMarconato commentedon Jan 9, 2024
@aeisenbarth thanks for reporting!
@giovp @kevinyamauchi @melonora I think we should prevent the same object to be added twice, what do you think? There were discussions on allowing the same object in-memory to be present in multiple locations of a
SpatialDataobject and to change the io functions to save it only once. This would have helped with an earlier design of the multiple tables. But with the current design I can't really see a benefit and probably we should just drop this complexity, at least for now.WDYT?
melonora commentedon Jan 9, 2024
I agree, I don't see a benefit of it either, just added complexity.
giovp commentedon Jan 16, 2024
mmh I really don't know though, I think this should be checked before and not with the check in #427 . Either a SpatialData can be instantiated with copies of elements or it can't. AnnData allows it for instance, so not sure why we shouldn't. I think on the contrary there is additional complexity on checking for the same element being present, as it should be done for sure at instantiation/IO and maybe in other parts (e.g. in subset ?).
LucaMarconato commentedon Jan 16, 2024
I agree, it should be checked before, I'd say at instantiation (constructor and
init_from_elements) and when the user uses the syntaxessdata.images['xxx'] = xxx. I'd still keep it also inlocate_element()as a double check for safety, to make sure that the result is not ambiguous.melonora commentedon Jan 17, 2024
@giovp What is the reason in AnnData for allowing it? Coming back to think about this, I would not see a use case personally right away. On the other hand what would be the problem (other than what @aeisenbarth reported currently) if the name of the element is different?
giovp commentedon Jan 17, 2024
I don't think checking that the element value is unique is something we should enforce, I don't see a use case but also I think it creates quite some constraints. Why not just returning a list of str in
locate_elementin #427 , what's the issue?melonora commentedon Jan 17, 2024
yes, I agree. It is what I tried to point at as the only problem currently is that
Falsegets returned. If a user has added one element twice with different names it is an intention of the user. We can just return the corresponding keys.LucaMarconato commentedon Jan 17, 2024
Ok, I have changed
locate_element()and to return an optional list of strings.aeisenbarth commentedon Jan 18, 2024
Semantically, an empty list of strings equally expresses "not found", just like None. But it is a simpler type, and one needs less code to work with the return value.
Using current implementation:
Using empty list:
LucaMarconato commentedon Jan 18, 2024
Very true, I was still focused on the older approach, gonna make a quick patch now.