Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC validation of NGFF with pydantic_ome_ngff #281

Closed
wants to merge 2 commits into from

Conversation

will-moore
Copy link
Member

See #258.

This PR tests one possible approach for using https://github.com/JaneliaSciComp/pydantic-ome-ngff/ in ome-zarr-py:

  • Uses appropriate class to parse JSON data when reading NGFF data

However, this doesn't use pydantic-ome-zarr when writing data yet, as the integration there will likely have to be much closer

@will-moore
Copy link
Member Author

E.g. failure on ubuntu with py3.10

pydantic/class_validators.py:304: in pydantic.class_validators._generic_validator_cls.lambda4
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'pydantic_ome_ngff.v04.multiscales.MultiscaleDataset'>
transforms = [VectorScaleTransform(type='scale', scale=[1.0, 1.0, 0.5, 0.18, 0.18])]

    @validator("coordinateTransformations")
    def check_transforms_types(
        cls,
        transforms: List[
            Union[ctx.VectorScaleTransform, ctx.VectorTranslationTransform]
        ],
    ) -> List[Union[ctx.VectorScaleTransform, ctx.VectorTranslationTransform]]:
        if (tform := transforms[0].type) != "scale":
            raise ValueError(
                f"""
            The first element of coordinateTransformations must be a scale transform.
            Got {tform} instead.
            """
            )
    
>       if (tform := transforms[1].type) != "translation":
E       IndexError: list index out of range

.tox/py310/lib/python3.10/site-packages/pydantic_ome_ngff/v04/multiscales.py:57: IndexError

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (685eb89) 84.84% compared to head (04fc4d4) 84.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   84.84%   84.88%   +0.04%     
==========================================
  Files          13       13              
  Lines        1485     1489       +4     
==========================================
+ Hits         1260     1264       +4     
  Misses        225      225              
Impacted Files Coverage Δ
ome_zarr/reader.py 86.84% <100.00%> (+0.13%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@will-moore
Copy link
Member Author

With JaneliaSciComp/pydantic-ome-ngff#7 merged and released as https://github.com/JaneliaSciComp/pydantic-ome-ngff/releases/tag/v0.2.3 tests are passing except for python 3.8 which isn't supported by pydantic-ome-zarr.

@will-moore will-moore closed this May 23, 2023
@joshmoore
Copy link
Member

@will-moore: what were your plans here? Is this viable? Waiting on something/someone else?

@will-moore
Copy link
Member Author

I guess there's a few points to discuss... - You made a good checklist at #258 (comment) which is mostly still outstanding.

  • Do we want to introduce this dependency on pydantic-ome-ngff yet? Cost/benefit?
  • pydantic-ome-ngff requires python = "^3.9". Would we want to handle python 3.8 by optionally not installing pydantic-ome-ngff. Or we drop 3.8 support. Or we try add 3.8 support to pydantic-ome-ngff?
  • The usage of pydantic-ome-ngff is very minimal currently (just in the reading of NGFF). This is probably where it's most useful, Do we stop there and only use it when reading or should we look at using it when writing NGFF too?
  • pydantic-ome-ngff only supports v0.4 OME-Zarr. It's possible we might want to release v0.5 support in ome-zarr-py before it's supported in pydantic-ome-ngff. Are we OK with that possibility?

@clbarnes
Copy link

clbarnes commented Jul 9, 2023

Or we drop 3.8 support

Numpy has already dropped 3.8 in new releases; most of the data ecosystem follows suit. pydantic-ome-ngff does now have "latest", which I believe is the current spec (not fair to expect 0.5 support when 0.5 hasn't been released yet...).

IMO migrating pydantic-ome-ngff to pydantic 2 should happen first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants