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

Image reading entry point #71

Merged
merged 11 commits into from
Mar 10, 2023
Merged

Image reading entry point #71

merged 11 commits into from
Mar 10, 2023

Conversation

ziw-liu
Copy link
Collaborator

@ziw-liu ziw-liu commented Feb 24, 2023

This PR removes ImageReader (previously WaveorderReader) as the extra abstraction layer, and replaces it with a top-level imread function that returns a instance of a children of ReaderBase. API of the reader object would stay the same, except for the simplification from what would previously be reader.reader.attr to reader.attr.

Start to address #40.

@ziw-liu ziw-liu mentioned this pull request Feb 24, 2023
@AhmetCanSolak
Copy link
Contributor

great starting point and it addresses certain aspects of #40 but this will not be enough to close it.

Signed-off-by: Ziwen Liu <67518483+ziw-liu@users.noreply.github.com>
@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Feb 25, 2023

great starting point and it addresses certain aspects of #40 but this will not be enough to close it.

Thanks! I removed #40 from the auto-closing list. This might actually help keep this PR smaller than a full-scale refactor.

@mattersoflight
Copy link
Collaborator

mattersoflight commented Feb 25, 2023

The imread interface is intuitive. I suggest testing this interface via convert CLI that converts some example datasets of each type into ome-zarr. I think you have come across all TIFF formats, but not PTI zarr format. You can find multiple example datasets here: https://zenodo.org/record/5951978#.Y_mKt7TMKrM.

I find that the current info CLI has some bugs. I reported in #21.

iohub/reader.py Outdated Show resolved Hide resolved
Pycromanager is no longer a dependency, and MM can also write NDTIFF.

Signed-off-by: Ziwen Liu <67518483+ziw-liu@users.noreply.github.com>
@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Feb 28, 2023

I suggest testing this interface via convert CLI that converts some example datasets of each type into ome-zarr.

To keep PRs atomic, I suggest that we implement the convert feature and its CLI option in a separate PR focused on #41.

@ziw-liu ziw-liu marked this pull request as ready for review February 28, 2023 01:09
@ziw-liu ziw-liu added the enhancement New feature or request label Feb 28, 2023
@mattersoflight
Copy link
Collaborator

mattersoflight commented Feb 28, 2023

To keep PRs atomic, I suggest that we implement the convert feature and its CLI option in a separate PR focused on #41.

👍🏼 the entry points can be tested and refined while working on #41.

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Mar 1, 2023

@mattersoflight I took a look at uPTI datasets. They seem to be exactly OME-Zarr stores except for extra calibration arrays in raw datasets (not in processed datasets). So if imread is called upon a uPTI store, it can load the positions as arrays with the generic v0.1 OME-Zarr reader. Dispatching to UPTIReader however is tricky, in that there is not a consistent pattern in file names or metadata to determine if a store is a uPTI dataset. Can you point me to the specification of this file format so I can learn more?

Edit: a work around is to dispatch to UPTIReader when data_type='upti' is specified in imread(). But this is more manual.
Edit: ⬆️ this is the current behavior. I had some confusion about what the UPTIReader aims to achieve: I thought it also handles Zarr, but it only handles raw-raw data in TIFF, not the '_raw' datasets in the Zenodo link.

@ziw-liu ziw-liu added this to the 0.1.0 milestone Mar 7, 2023
iohub/multipagetiff.py Show resolved Hide resolved
iohub/reader.py Show resolved Hide resolved
iohub/reader.py Show resolved Hide resolved
iohub/reader_base.py Show resolved Hide resolved
@mattersoflight
Copy link
Collaborator

mattersoflight commented Mar 9, 2023

They seem to be exactly OME-Zarr stores except for extra calibration arrays in raw datasets (not in processed datasets). So if imread is called upon a uPTI store, it can load the positions as arrays with the generic v0.1 OME-Zarr reader.

That's awesome!

I had some confusion about what the UPTIReader aims to achieve: I thought it also handles Zarr, but it only handles raw-raw data in TIFF, not the '_raw' datasets in the Zenodo link.

Thanks for that clarification. I'd forgotten how the code evolved. The data acquired with PTI is indeed in TIFF format.

Examples: CompMicro/rawdata/hummingbird/LiHao/20210120_A549_RSV_20x and CompMicro/rawdata/hummingbird/LiHao/20210226_uPTI_20x_Zebrafish.

Please test if the current reader can convert this data or not. Great if that works. We can handle any fixes in a separate PR. We also need to update documentation and refactor code in waveOrder to depend on iohub to enable data i/o for PTI.

@ziw-liu
Copy link
Collaborator Author

ziw-liu commented Mar 10, 2023

Please test if the current reader can convert this data or not.

@mattersoflight Just a reminder that this PR does not feature a converter. That's to be added in #76.

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