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

Universal reader output base classes #132

Merged
merged 18 commits into from
Jun 13, 2023
Merged

Conversation

JoOkuma
Copy link
Member

@JoOkuma JoOkuma commented May 18, 2023

Hi, this is a work in progress for the base classes so we can advance with the universal reader.

I'm happy to iterate and get feedback from every to have a friendly interface.

I think we could be more restrictive than ome-zarr in some aspects; this would reduce the number of edge cases we need to worry about when implementing our own pipelines (e.g. fixing the shape to 5 and 6)

@JoOkuma
Copy link
Member Author

JoOkuma commented May 18, 2023

@ziw-liu I reviewed what you mentioned about the __contains__, __delitem__ and other dictionary related methods.

This might be a case where the ClearControl (maybe TIFF too) API differs from the NGFFNode, it behaves more like the ImageArray.

I need to think a bit more about this, from my understanding a zarr FOV is NGFFNode because the ngff spec is not very rigid (the arrays can have arbitrary names) and the multi-scales, right?

@ziw-liu
Copy link
Collaborator

ziw-liu commented May 18, 2023

@JoOkuma We don't need __setitem__ and __delitem__ since these are read-only.

Also just to clarify I was talking about BaseFOVCollection. So we can do bool("name" in SomeFOVCollection()).

For BaseFOV I think it might be ok to ignore pyramids for the time being. (In theory TIFFs can have pyramids too.)

iohub/dataset_base.py Outdated Show resolved Hide resolved
@ziw-liu ziw-liu added the enhancement New feature or request label May 18, 2023
@JoOkuma
Copy link
Member Author

JoOkuma commented May 18, 2023

@JoOkuma We don't need __setitem__ and __delitem__ since these are read-only.

Also just to clarify I was talking about BaseFOVCollection. So we can do bool("name" in SomeFOVCollection()).

For BaseFOV I think it might be ok to ignore pyramids for the time being. (In theory TIFFs can have pyramids too.)

Got it, I updated it accordingly.

iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
JoOkuma and others added 2 commits May 22, 2023 13:39
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
@ziw-liu
Copy link
Collaborator

ziw-liu commented May 22, 2023

Also we should figure out how the consequent PRs should merge. It's fine if this one merges into main but if we keep the refactoring of readers atomic and distributed, asynchronously merging into main will result in a broken default branch.

Maybe make a new target branch (e.g. uapi-readers) once this is merged?

@JoOkuma
Copy link
Member Author

JoOkuma commented May 23, 2023

Also we should figure out how the consequent PRs should merge. It's fine if this one merges into main but if we keep the refactoring of readers atomic and distributed, asynchronously merging into main will result in a broken default branch.

Maybe make a new target branch (e.g. uapi-readers) once this is merged?

I like the idea of having a separate branch.

I'm happy to take a few days next week and be done with it. I'm free after next Wednesday, but I'll need help with tiff because I'm not super familiar with it.

@ziw-liu ziw-liu mentioned this pull request May 31, 2023
5 tasks
@JoOkuma JoOkuma mentioned this pull request Jun 6, 2023
Copy link
Contributor

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

This all LGTM! Thank you guys. I like the BaseCollectionFOV as an iterable that we can use to access individual positions.

A couple of points just to make sure this will behave as intended.

  • For this universal reader, we will call BaseFOV[0,0,0] this will return a (1,1,1,Z,Y,X) 5D array?

@ziw-liu
Copy link
Collaborator

ziw-liu commented Jun 8, 2023

  • For this universal reader, we will call BaseFOV[0,0,0] this will return a (1,1,1,Z,Y,X) 5D array?

My understanding is that it will behave as normal array slicing does. So YX (2D) in this case.

Edit: assuming you meant BaseFOV[0,0,0] instead of BaseFOV[0,0]

iohub/dataset_base.py Outdated Show resolved Hide resolved
@ziw-liu ziw-liu mentioned this pull request Jun 9, 2023
Copy link
Contributor

@talonchandler talonchandler left a comment

Choose a reason for hiding this comment

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

Looks great to me! I'm excited.

Thanks for pushing on this @JoOkuma.

iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
iohub/dataset_base.py Outdated Show resolved Hide resolved
@JoOkuma
Copy link
Member Author

JoOkuma commented Jun 12, 2023

@ziw-liu, I renamed files from database_base.py to fov.py.

iohub/fov.py Show resolved Hide resolved
@ziw-liu ziw-liu changed the title WIP: universal reader output base classes Universal reader output base classes Jun 13, 2023
iohub/fov.py Outdated Show resolved Hide resolved
iohub/fov.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ziw-liu ziw-liu left a comment

Choose a reason for hiding this comment

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

Looks good!

@JoOkuma JoOkuma merged commit aabfd2f into czbiohub-sf:main Jun 13, 2023
@JoOkuma
Copy link
Member Author

JoOkuma commented Jun 13, 2023

Thanks y'all.

This was referenced Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants