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

Refactor NGFF module and migrate to Pydantic v2 #233

Merged
merged 18 commits into from
Jul 29, 2024
Merged

Refactor NGFF module and migrate to Pydantic v2 #233

merged 18 commits into from
Jul 29, 2024

Conversation

ziw-liu
Copy link
Collaborator

@ziw-liu ziw-liu commented Jul 16, 2024

(Breaking) Moved the NGFF files to a subpackage.

Migrate to Pydantic v2. Downstream packages need to use the v1 namespace as shown in mehta-lab/recOrder@13ca75e.

This PR is laying the ground work for the upcoming NGFF UAPI implementation. But the diff is significant enough so I'd like to get it reviewed first.

@ziw-liu ziw-liu marked this pull request as draft July 16, 2024 17:33
@ziw-liu ziw-liu changed the title Refactor NGFF files and migrate to Pydantic v2 Refactor NGFF module and migrate to Pydantic v2 Jul 16, 2024
@ziw-liu ziw-liu marked this pull request as ready for review July 16, 2024 18:16
@ziw-liu ziw-liu requested review from JoOkuma and ieivanov July 16, 2024 18:17
@ziw-liu ziw-liu added NGFF OME-NGFF (OME-Zarr format) maintenance Maintenance work labels Jul 16, 2024
@ziw-liu ziw-liu added this to the 0.2.0 milestone Jul 16, 2024
@ziw-liu ziw-liu linked an issue Jul 16, 2024 that may be closed by this pull request
@JoOkuma
Copy link
Member

JoOkuma commented Jul 17, 2024

@ziw-liu should you update the dependencies to pydantic>=2?

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jul 17, 2024

@ziw-liu should you update the dependencies to pydantic>=2?

@JoOkuma I'm not sure why 8ac39c7 doesn't show up in diff.

nvm it's a github glitch. You should see it now.

Copy link
Member

@JoOkuma JoOkuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much to add, great PR. The improved typing is much appreciated.

iohub/ngff/models.py Outdated Show resolved Hide resolved
iohub/ngff/nodes.py Show resolved Hide resolved
ziw-liu and others added 2 commits July 19, 2024 10:27
@ziw-liu ziw-liu requested a review from JoOkuma July 22, 2024 18:01
Copy link
Contributor

@ieivanov ieivanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, free free to merge when you're happy with it. Somewhere could you give more details on what belongs in ngff.models, ngff.nodes, and ngff.display? Is there a parallel to structures in the TIFF data readers? With the Universal API, do you think we can also achieve similar code structure?

@ieivanov ieivanov mentioned this pull request Jul 22, 2024
4 tasks
@JoOkuma
Copy link
Member

JoOkuma commented Jul 29, 2024

@ziw-liu sorry, I forgot to approve after my review.

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jul 29, 2024

Somewhere could you give more details on what belongs in ngff.models, ngff.nodes, and ngff.display?

See f718d73.

Is there a parallel to structures in the TIFF data readers? With the Universal API, do you think we can also achieve similar code structure?

What structure are you envisioning? I organized NGFF files here so that on the top level (from iohub import ...) every file format gets a single namespace. Since the other formats have much less LOC I think they can be kept in a single file as is.

@ziw-liu ziw-liu merged commit 885661d into main Jul 29, 2024
7 checks passed
@ziw-liu ziw-liu deleted the refactor-ngff branch July 29, 2024 20:00
edyoshikun pushed a commit that referenced this pull request Jul 31, 2024
* unify comment format

* fix typing and docstring

* create ngff sub-package
and refactor display util file

* refactor ngff meta file

* refactor ngff

* export transformation model

* fix type hint

* bump ome-zarr target in docstring

* migrate to pydantic v2

* isort

* fix validators

* remove union type

* fix dependency

* update docstring

* typing improvements

* Update iohub/ngff/models.py

Co-authored-by: Jordão Bragantini <[email protected]>

* fix style

* update module docstring to specify their content

---------

Co-authored-by: Jordão Bragantini <[email protected]>
ieivanov added a commit that referenced this pull request Sep 27, 2024
* initial commit with added docs prior to refactoring to simplify the naming and functions.

* considering varying t_idx_in and out

* template for hypothesis

* Refactor NGFF module and migrate to Pydantic v2 (#233)

* unify comment format

* fix typing and docstring

* create ngff sub-package
and refactor display util file

* refactor ngff meta file

* refactor ngff

* export transformation model

* fix type hint

* bump ome-zarr target in docstring

* migrate to pydantic v2

* isort

* fix validators

* remove union type

* fix dependency

* update docstring

* typing improvements

* Update iohub/ngff/models.py

Co-authored-by: Jordão Bragantini <[email protected]>

* fix style

* update module docstring to specify their content

---------

Co-authored-by: Jordão Bragantini <[email protected]>

* improving docstring for functions and renaming input and output path arguments to be more intuitive.

* Fixing pyramid scaling factor (#238)

* Fixing pyramid scaling factor

* fix pyramid test

* fix test again

* Use annotation instead of field for tagged union (#244)

also removed non-standard axis

* Export `ImageArray` from the `ngff` module (#245)

* adding the ImageArray

* precommit

* black

* renaming method arguments to have consistent naming structure

* flake8

* refactor _calculate_zyx_chunk_size

* use input_store_path and output_store_path throughout

* style

* rename and clean up time indices

* update time_indices documentation

* add processing for channel indices

* fix syntax and move ngff_utils.py to ngff/utils.py

* update import

* typing

* docs typos

* fix process_single_position iterator @talonchandler @edyoshikun

* update apply_transform... docstring

* compatibility with minimal deskew w/ @edyoshikun

* pretty flat_iterable

* adding new tests

* create_empty test without testing channel names

* fixing the create_empty_zarr extra indentation

* -attempt to fix apply_transform_test. @ieivanov revert if needed

* fixed apply_transform_czyx

* debug pytest

* fixing the test for create_empty_plate pytest

* synchronize log messages

* docs improvements

* improved docs and typing for Callable func

* remove commented slurmkit fix

* delete unused function

---------

Co-authored-by: Ziwen Liu <[email protected]>
Co-authored-by: Jordão Bragantini <[email protected]>
Co-authored-by: Ivan Ivanov <[email protected]>
Co-authored-by: Talon Chandler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance work NGFF OME-NGFF (OME-Zarr format)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pydantic 2.0
3 participants