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 API for NGFF datasets #31

Merged
merged 111 commits into from
Feb 13, 2023
Merged

Refactor API for NGFF datasets #31

merged 111 commits into from
Feb 13, 2023

Conversation

ziw-liu
Copy link
Collaborator

@ziw-liu ziw-liu commented Jan 27, 2023

Implements #19.
Fixes #29.

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Jan 27, 2023

At this point only iohub.ngff.OMEZarr works. (See examples/ome_zarr_writer.py and docstrings for intended usage patterns.)

Again early testing and suggestions are most welcome!

@ziw-liu ziw-liu added enhancement New feature or request NGFF OME-NGFF (OME-Zarr format) labels Jan 27, 2023
@ziw-liu ziw-liu self-assigned this Jan 27, 2023
@ziw-liu ziw-liu added this to the 0.0.1 milestone Jan 27, 2023
Copy link
Collaborator

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

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

Great iteration! The open_ome_zarr method with layout parameter is intuitive!
I have renamed & edited examples for clarity. Comments below are related to the following points.

  • Repurpose the nomenclature from our dependencies:
  • Accessing the 5D arrays (zarr arrays) via position['key'] syntax is not intuitive. Let's implement position.data (set or get property) syntax, which mimics napari's syntax.

  • Rename the ImageArray object to ZarrArray to make it obvious that the object is a zarr array.

  • Make it intuitive to read and write tiled acquisitions:
  • Introduce a tiles or tiled layout. I picked these words in favor of multiPos or multiFOV. But, I think either of them is a decent choice.
  • Make it intuitive to over-write channels:
  • setitem (position[array_name][:, channel_index] = array) is too complex a usage, let's provide a wrapper delete_channel or overwrite_channel. delete_channel is more broadly useful.

iohub/ngff.py Show resolved Hide resolved
iohub/ngff.py Outdated Show resolved Hide resolved
iohub/ngff.py Show resolved Hide resolved
@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Feb 11, 2023

@mattersoflight Thanks for the detailed review. At this point I think this PR has grown too large (100+ commits) and further refactoring makes it difficult to trace the history. I'm tempted to move your comments/edits to a new issue/branch pair and merge this into main at 6657abb.

@mattersoflight
Copy link
Collaborator

mattersoflight commented Feb 11, 2023

@mattersoflight Thanks for the detailed review. At this point, I think this PR has grown too large (100+ commits) and further refactoring makes it difficult to trace the history. I'm tempted to move your comments/edits to a new issue/branch pair and merge this into main at 6657abb.

You're welcome! If you want to take up improvements in API into a separate issue and PR, feel free.
Just refine the three examples (single FOV, hcs, tiled zarrs) with the current API and include them in this PR, starting at 61237f9

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Feb 13, 2023

Moved commits after 6657abb to ngff-api.

This reverts commits 6d0ba4c,
b9516b1, 61237f9.
@ziw-liu ziw-liu mentioned this pull request Feb 13, 2023
3 tasks
@ziw-liu ziw-liu merged commit 4a9e6fc into main Feb 13, 2023
@ziw-liu ziw-liu deleted the ngff-dataset branch February 13, 2023 22:08
ziw-liu pushed a commit that referenced this pull request Mar 3, 2023
ziw-liu added a commit that referenced this pull request Mar 14, 2023
* Merge pull request #14 from mehta-lab/QLIPP_pipeline

recOrder pipeline infrastructure + qlipp pipeline submodule

* Merge pull request #22 from mehta-lab/zarr_converter

Zarr converter

* Merge pull request #52 from mehta-lab/fluor_deconv

Fluorescence Deconvolution and PhaseFromBF pipelines

* Merge pull request #31 from mehta-lab/calibration_plugin

Calibration/Acquisition Plugin

* Merge pull request #112 from mehta-lab/cleanup-tests-and-readme

Improved testing, configs, and dev tools

* Merge pull request #123 from mehta-lab/zarr-converter-position-bug

Fix ome-tif to zarr converter for acquisitions with micromanager beta

* Merge pull request #140 from mehta-lab/restructure_tests

Restructure tests

* Merge pull request #148 from mehta-lab/pycromanager_converter_v2

Pycromanager converter v2

* Merge pull request #167 from mehta-lab/bkg-correction-fixes2

Fix background correction for non-square images and online mode

* Merge pull request #157 from mehta-lab/RAM-warning

Print RAM warning message in online and offline modes

* Merge pull request #169 from mehta-lab/warn-bkg-averaging

Remove background averaging

* Merge pull request #174 from mehta-lab/show-ram-warning

Show RAM warning in GUI / log in CLI

* Merge pull request #175 from mehta-lab/better-messages

Improved warning message

* Merge pull request #181 from mehta-lab/error-mismatched-bkg-and-img

Always load background and warn if background and image sizes are mismatched

* Merge pull request #182 from mehta-lab/error-mismatched-bkg-and-img

Mismatched x/y size of background and acquisition should error instead of warn

* Merge pull request #210 from mehta-lab/0.2.0-gui-hide

Remove fluorescence, preprocessing, and postprocessing; simplify GUI

* Merge pull request #219 from mehta-lab/0.2.0-integration

0.2.0 integration PR

* Introduce Black formatting into the code base (#229)

* black-format all `.py` files except recorder_ui.py

* create .git-blame-ignore-revs

* Document how to ignore formatting commits

* Document `git` version

Co-authored-by: Talon Chandler <[email protected]>

* draft refactor for imread

* expose primary entry points

* fix type check and tests

Signed-off-by: Ziwen Liu <[email protected]>

* fix formatting

* update `imread` docstring

* define supported formats

Signed-off-by: Ziwen Liu <[email protected]>

* rename pycromanager reader to ndtiff

Pycromanager is no longer a dependency, and MM can also write NDTIFF.

Signed-off-by: Ziwen Liu <[email protected]>

* remove 'doesnt work' code

* improve error messages

* rearrange files from patch

* depend on tqdm for progress bar in conversion

* move util function

* delete other util functions

* make util function private

* remove redundant file

* rename converter file

* fix some of the references and docstrings

* draft refactor

* write with new ngff module

* fix position grid

* fix image check

* close writer after conversion

* test ome-tiff conversion

* handle invalid stage position metadata

* support labelling positions

* fix typo

* fix path type

* convert single page tiff datasets

This is rather broken: the reader does not handle metadata well

* allow longer conversion time in tests

Signed-off-by: Ziwen Liu <[email protected]>

* draft cli command

Signed-off-by: Ziwen Liu <[email protected]>

* fix argument

* detect flat ndtiff

* sort imports

Signed-off-by: Ziwen Liu <[email protected]>

* limit bar length

* add -h shortcut for help

* test cli

* fix get_image

* test against a random choice of dataset

---------

Signed-off-by: Ziwen Liu <[email protected]>
Co-authored-by: Cameron Foltz <[email protected]>
Co-authored-by: Shalin Mehta <[email protected]>
Co-authored-by: Talon Chandler <[email protected]>
Co-authored-by: Ivan Ivanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NGFF OME-NGFF (OME-Zarr format)
Projects
None yet
5 participants