fix incremental update

[giovp]

starting to work on #293

this PR only partially address the issue, in particular it implements the following:

• enforce uniqueness of names of elements, by introducing the Elements class
• remove lazy vs. in-memory logic for adding elements.

[giovp]

heads up, had to introduce a new module 😬 called "elements" (happy to change name).
Thing is, the __setitem__ method should be consistent whether a new element is added directly to spatialdata, or to the elements dict.
e.g.

sdata = SpatiaData(images={"myimage":image})
sdata["myimage"] = image # raise error
sdata.images["myimage"] = image # did not raise error, now it does

I must say I'm not 100% sure this is expected behaviour but at least it is consistent, wdyt? @LucaMarconato

EDIT: lots of tests failing because of this

[giovp]

I've started with option 1 from zulip but now the following example doesn't throw warning

sdata = SpatiaData(images={"myimage":image}, labels={"mylabel":label})
sdata["myimage"] = newimage
>>> UserWarning
sdata.labels["myimage"] = newlabel # does not throw warning, but it should..

need to think of best option

[LucaMarconato]

@giovp still draft or ready for review?

[giovp]

still draft but very close!

[codecov]

Codecov Report

Merging #329 (859094c) into main (c620c45) will decrease coverage by 0.16%.
The diff coverage is 93.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
- Coverage   91.75%   91.59%   -0.16%     
==========================================
  Files          36       37       +1     
  Lines        5007     4997      -10     
==========================================
- Hits         4594     4577      -17     
- Misses        413      420       +7     

Files	Coverage Δ
src/spatialdata/_io/io_raster.py	96.29% <ø> (-0.07%)	⬇️
src/spatialdata/_io/io_zarr.py	90.29% <100.00%> (ø)
src/spatialdata/_types.py	76.92% <100.00%> (+10.25%)	⬆️
src/spatialdata/_core/spatialdata.py	92.77% <95.08%> (-1.22%)	⬇️
src/spatialdata/_core/_elements.py	92.00% <92.00%> (ø)

[giovp]

@LucaMarconato @kevinyamauchi this is ready for review !

[melonora]

Just some minor questions / comments. Overall looks good to me and in fact a lot cleaner!

[melonora]

I would say a lot of people actually use this to store information that can't be stored in metadata yet. What is the reason for removing it currently?

[LucaMarconato]

I agree with @giovp in removing this, using a custom name has two downsides:

1. it makes the access to tensor inside the xarray object more verbose and non uniform, for instance one needs to write something like sdata['my_image_name']['scale0']['my_image_name'], instead of the standardized sdata['my_image_name']['scale0']['image'].
2. if the user assignes the image to a different name, like with sdata['my_image2'] = sdata['my_image_name'], the internal name will still renamin the old one, which is confusion.

@giovp for the sake of brevity I would actually call the name im instead of image; also in the case of labels I think im sounds better than image.

[melonora]

Not related to the code but I believe this test would fail on a windows system. We currently do not do CI for windows, but for shapes here only np.int64 is supported while on windows this creates np.int32 dtype.

Shall I open an issue for this?

[LucaMarconato]

Yes, please opena an issue for this; we should consider adding a Windows CI at some point in the next months.

[melonora]

just for others after some small QA with Giovanni, in case that the scope of this PR is to also allow for partial writes to disk, this should still be implemented as I believe this PR does not do this yet. In case it is implemented it is currently not covered by the tests yet.

[melonora]

Also upon testing with napari spatialdata, the userwarning is given every time you click on a coordinate space. I will look into it what causes this behaviour.

[kevinyamauchi]

LGTM. I like the additional Elements class. This could be useful for shimming other actions and validators in the future. Thanks!

[kevinyamauchi]

Oops, last review was meant to be an approval

[LucaMarconato]

I would like to know if there are any downsides to actually using a class attribute here for _shared_keys. Also more from the perspective of clarity, would a name like _existing_keys be more clear? The keys are not actually shared, in fact they should not be shared as they should be unique.

I agree, I would rename to _existing_keys, wdyt @giovp?

[LucaMarconato]

Let's keep in mind that this PR removed the commented code here, since it will probably be useful for making the "lazy copy" approach, or similar, work in the future.

[LucaMarconato]

This should be removed. It's the residual of some old code was for reloading lazily those data that could be lazy loaded. It's better for the user to reload things explicitly, maybe. I say maybe because certain softwares, for instance illustrator, change the path the current session is pointing to whenever the file is saved.

[LucaMarconato]

I have a comment on this. See GitHub discussion.

[LucaMarconato]

I think we shuold replace this with self._path = Path(file_path)

[LucaMarconato]

I have a comment on this setter, not sure if we should disallow it. See GitHub discussion. (I added a similar "See GitHub discussion" comment in other places, they all refer to the same discussion.)

[LucaMarconato]

Yes, I think this setter should not be public.

[LucaMarconato]

Let's remember that this code was removed here because we will need to add similar tests when working with incremental IO or with lazy copy.

[LucaMarconato]

Thanks @giovp for the PR, the code reads much cleaner and is more robust now! All the comments that I have added above are minor and can be very quickly resolved. The only part that requires some extra attention is what I will discuss in this comment and is about the meaning of sdata.path. Anyway, also this comment has an easy fix (removing the setter for sdata.path) and then we can follow up in the incremental-io PR.

In the previous PR, the sdata.path could either be None or a real path. In the first case the object could have been anything: a pure in-memory object or a mix of a in-memory/lazy-loaded/lazy-computed object. But if read() was called (and read() was automatically called after a write()), the path was then changed to a string, and this guaranteed that the lazy-loadable elements (images, labels and points), were necessarily being lazy loaded from the Zarr store.

In fact, if the user called read() and then sdata.add_points(...), the new points would not be added in-memory, but first written on disk and then lazy read. The advantage of this is the consistency of the meaning of sdata.path (I write it again for clarity: sdata.path is a str if and only if all the images, labels and points elements that are present are lazy loaded from the Zarr substores inside that path). But this came with a big disadvantage, the inability to overwrite lazy loaded data and enough to make the current PR necessary.

But this opens up the question of what's the meaning of sdata.path. I think we can postpone this question when we work on incremental IO, but for the moment I would say that we can do the following:

• we remove the public setter for sdata.path;
• sdata.path is automatically set when read() is called;
• the user after calling write() will probably often want to manually call read() again because otherwise the performance improvement given by Zarr multiscale and parquet are not available. We need to make this clear in an new example notebook to make the user aware. Finally the user will be able to load in-memory specific element so that some elements are lazy loaded and some of them are fully loaded, but again, let's postpone this discussion to the incremental-io PR.

Anyway, since all the comments are minor I am pre-approving the PR, great work! 🚀

[giovp]

thanks @LucaMarconato for the comment, I agree on the plan.

• I have fixed the setter behavior for images/labels/points/shapes and added tests
• I have removed the path setter and removed the re-reading after write of the elements.