Ome zarr scene - #612
Conversation
feat: add OMEZarrScene to __init__
|
Rebased on #605 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #612 +/- ##
==========================================
+ Coverage 86.46% 86.94% +0.47%
==========================================
Files 16 17 +1
Lines 2365 2574 +209
==========================================
+ Hits 2045 2238 +193
- Misses 320 336 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Both transformnd and ome-zarr-models-py in require |
|
Both ozmp and transformn use the TypeVar feature from Python 3.12 extensively, so dropping 3.11 is necessary if we want to use these pacakges in conjunction |
| def __init__( | ||
| self, | ||
| images: list[OMEZarrMultiscale] | dict[str, OMEZarrMultiscale], | ||
| coordinate_transformations: Sequence[AnyTransform] | list[dict[str, Any]], |
There was a problem hiding this comment.
Can't you just accept only the Sequence[AnyTransform]? I don't think we gain much by having the constructor perform this kind of conversion internally. If a client has raw dicts, they can parse it themselves and handle failures however they find appropriate.
There was a problem hiding this comment.
My motivation to support both was to shield users from having to deal with ome-zarr-models-py classes. If I accepted only Sequence[AnyTransform] here, that would have users need to go through
- reading and understanding the spec
- building the respective ome-zarr-models-py instance
- passing it to the scene
My original intention behind the class-based API was to shield/abstract the model class handling as much as possible, but I would also find it a bit overkill to not allow ozmp objects. So...accepting both seemed like a reasoanble trade-off.
There was a problem hiding this comment.
I don't think this shields the user from ozmp classes, because you are still using them internally, so users are implicitly expected to either know those classes or have read the raw spec to even be able to craft a raw dict that has the proper shape and types.
Further, if the user makes a mistake, an exception will be thrown further away from where the mistake was (i.e.: it will be thrown from within OMEZArrScene.__init__ instead of where the user created the parameters. Further, depending on how the ozmp parses the raw dicts, it's totally possible that a user might pass in some dict that has fields that are silently ignored while parsing them.
By only taking the ozmp models (or whatever other structured class you might want), the users can be naturally guided into doing the right thing: They come to your class, see that you need some ozmp objects. Then they can go to ozmp class definitions and try to create those, following the docs and type hints there. Once they managed to create those, they can come back to OMEZArrScene and look at the next argument type, and so on and so forth.
And if they are using some IDE or LSP that understands the type hints, they can tab-complete to discover fields, which they can't do on raw dicts.
There was a problem hiding this comment.
have read the raw spec to even be able to craft a raw dict that has the proper shape and types.
Jep. For the OMEZarrMultiscale and OMEZarrImage classes, I think they are designed now in such a way, that this is actually not needed. Where it comes to the transforms and the scene, writing an API around the transforms stuff that allows you do use the Scene class without also knowing about the underlying spec is much harder. So you're right here, you cannot build a scene transform graph without knowing how these are structured (yet).
Further, if the user makes a mistake, an exception will be thrown further away from where the mistake was
The error might fly in the init, but wouldn't you always find the traceback from the ozmp error at the end of that? Assuming that that's where users would look for the error, this would put the pressure on ozmp to throw informative warning messages, which I think is reasonable. Personally, I don't care so much which tool throws the error in how deep a traceback, as long as it shows me what I need to do/change.
Then they can go to ozmp class definitions and try to create those, following the docs and type hints there.
As the sole ozmp maintainer right now AND writer of the spec, I would rather write it very cleanly in the spec, link to the respective sections here and make sure that ozmp throws useful errors.
And if they are using some IDE or LSP that understands the type hints, they can tab-complete to discover fields, which they can't do on raw dicts.
THAT is a good point. I would argue that as a newcomer, your starting point would likely be the examples in the spec document and your workflow would be something like SomeClass.model_validate(some_dict), for which you'd also start with the dict.
The other argument I'd bring against narrowing ourselves to ozmp objects is something I wrote below: I don't want to promote the choice of ozmp as a model backend to be part of the API just yet. There are other libraries for that, too, If we decide at some point to swap out the model implementation to reduce duplicity, I wouldn't want to change the API, too.
| super().__setattr__(name, value) | ||
|
|
||
| @staticmethod | ||
| def _parse_transforms( |
There was a problem hiding this comment.
Again I'd argue that clients should know what they have in hand, and if they need to parse some raw dicts, they should do it themselves
There was a problem hiding this comment.
My argument would be that the intersection of people with Python + Pydantic + spec knowledge is smaller than that of Python + spec knowledge. So if you are versed with ome-zarr-models-py instances that's fine and you can pass these, but I wouldn't want to make it a pre-requisite.
The other thing is that exposing this choice would make it much harder for us in the future to swap out the model backend, if we ever chose to. For instance, yaozarrs is pretty good in that respect and I wouldn't want a choice of a different model backend lead to a completely different API.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This reverts commit e0a03cd.
This reverts commit 75b9402.
This reverts commit b12fb12.
This reverts commit dcd3858.
for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
e809c82 to
ad8ea3c
Compare
c0cbd9c to
529b9cf
Compare
d51f2ff to
9f851e1
Compare
73ec05e to
39c9e12
Compare
|
@will-moore @kevinyamauchi @melonora @Tomaz-Vieira I think this is as good as it gets if you want to give it another look? |
Clean version of #583. The commit history there is a bit garbled up, especially since I rebased #515 a few times before merging and #583 was forked off that branch earlier.
New: OMEZarrScene
Builds upon the
OMEZarrMuzltiscaleclass framework. Provides the following methods:from_ome_zarr(): Read a zarr group into a scene class instance to access transforms, coordinate systems and contained ome zarr images easily through the class-based APIto_ome_zarr(): Serialize an ome-zarr scene to disk, with the option to append already existing scenes if writing would be too heavy otherwiseAttributes:
images: Dict of essentially{group: image}, whereimageis an instance ofOMEZarrMultiscalecoordinate_transformations: List of ome-zarr-models-py transform instancescoordinate_systemsList of coordinate systems in thescenemetadata (if they exist)coordinate_displacements: Same likeimages: Dict that lists stored coordinates or displacements arrays.Features:
scene._graph)but I could imagine exposing it more in the future if we come up with a neat API for it