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

Update NDTIFF reader #145

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Update NDTIFF reader #145

merged 1 commit into from
Jun 20, 2023

Conversation

ziw-liu
Copy link
Collaborator

@ziw-liu ziw-liu commented Jun 8, 2023

Resolves #124.

This bumps ndtiff to 2.1.0 so that we can use the automatic axes sorting instead of pulling our own.

Currently CI only tests for NDTIFFv2 datasets. @ieivanov Should we add some v3 datasets to the pool?

@ziw-liu ziw-liu added μManager Micro-Manager files and metadata maintenance Maintenance work labels Jun 8, 2023
@ziw-liu ziw-liu marked this pull request as ready for review June 8, 2023 21:52
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.

Haven't tested, but I think the code looks good

Comment on lines 140 to 142
# TODO: try casting the dask array into a zarr array
# using `dask.array.to_zarr()`.
# Currently this call brings the data into memory
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe now as_array returns a dask array without bring the data into memory, I think you can test and remove this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this means that dask.array.to_zarr() brings data to memory if the store is a MemoryStore so this method cannot be consistent with other get_zarr() methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully this won't be a problem anymore after #132 and we refactor this.

@ieivanov
Copy link
Contributor

ieivanov commented Jun 9, 2023

Should we add some v3 datasets to the pool?

Yes, I can acquire such datasets. Where would we store them? Should we update the waveorder collection or create a new one?

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jun 9, 2023

Should we update the waveorder collection or create a new one?

Could work either way. If we do create a new one we could also migrate the existing test datasets over too.

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jun 12, 2023

@ieivanov do you think we should merge this as-is or wait for new test data?

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jun 20, 2023

Opened #149 to track the test dataset issue. Merging.

@ziw-liu ziw-liu merged commit 6cc6683 into main Jun 20, 2023
@ziw-liu ziw-liu deleted the ndtiff-shape branch June 20, 2023 18:32
JoOkuma added a commit that referenced this pull request Jun 21, 2023
* inheriting basefov on ccfov

* add deprecation warning to ccfov.scale

* activating github pr workflow

* Update NDTIFF reader (#145)

use new ndtiff and stop sorting axes

* add ccfov scales tests

* Release timing requirement for I/O-heavy test (#147)

* release timing requirement for I/O-heavy test

* suppress data size check for arrays

---------

Co-authored-by: Ziwen Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance work μManager Micro-Manager files and metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bump ndtiff version to >1.12.1
2 participants