-
Notifications
You must be signed in to change notification settings - Fork 28
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
OME-TIFF Read / Write #819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@cliu72 I've added a short utility notebook which walks a user through the conversion process. Let me know your thoughts! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I think it would be good to include in the notebook description that the conversion is to single-channel TIFs, instead of ambiguously saying it's in a format for ark. Maybe something like: "This notebook is used to convert an OME-TIFF into single-channel TIFFs (one TIFF per marker), which is the format used throughout Ark-Analysis."
Also, I think it would be good to have a more descriptive title for the notebook - like "OME-TIFF_Converter" or something like that?
Is there also going to be a notebook going the other way (i.e. single channel -> OME), or is that going to be a task for later? (I only need this one for my paper so I don't really care lol)
…786) * Initial commit of replacing _feature_0 with _whole_cell and _feature_1 with _nuclear * Make sure dataset tests include _whole_cell and _nuclear * Update distance matrix column to be dist_whole_cell * Patch up marker quantification loading * Ensure channels for stitch_images test are being passed in correctly * Rename segmentation label saving to _whole_cell and _nuclear * Define both the whole cell and nuclear suffix renaming choices for create_deepcell_output * Change revision to match current PR on HuggingFace with new suffix names * Patch up calc_dist_matrix suffix * PYCODESTYLE in data_utils.py * Patch up deepcell_utils * Use new HuggingFace commit hash without hidden old files * Change legacy example_dataset extensions to _whole_cell.tiff * Make sure neighborhood mask uses _whole_cell.tiff * Update comment to _whole_cell.tiff too * Aesthetics * Patch _feature_0 to _whole_cell to pass neighborhood mask generation test * Change suffix in the README * Refer to main branch for HuggingFace
renamed utility notebook
@cliu72 I fixed the issue for reading the second image. You need the
Also the Example Dataset does not include any OME-TIFFS, would you like a few of those in for this converter notebook in particular? |
If it's not too much work, can we add an example OME-TIFF? I think you can just take one of the example FOVs, use this notebook to create the OME-TIFF, and then add it to the example dataset. I think just one FOV would be fine. And then can we move the cell to download the example dataset to the relevant sections? I.e. move the |
@cliu72 Dataset is available on this branch, Need to fix a couple of tests, but you can use it with the conversion notebook. |
Sweet, looks good! Last thing, can we add more description to before the cell to download the example dataset? Maybe so it looks like this: OME-TIFF to Single Channel TIFFsIf you would like to test this feature using an example dataset, run the cell below. To use your own data, skip the cell below and change
[rest of the code] Single Channel TIFFs to OME-TIFFIf you would like to test this feature using an example dataset, run the cell below. To use your own data, skip the cell below and change [rest of the code] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there are remaining merge/example dataset issues to resolve, but I think this looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some merge conflicts that need to be addressed. It will also change the requirements, which will require a new docker. We'll discuss at our meeting today if it makes sense to get all that done today, or wait till after the closure
@ngreenwald Missing some coverage, as I'm not able to create ome-tiffs with tifffile that have a slightly different format (i.e. a couple of the example ones that @cliu72 tested out). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but I think we should hold off on adding TMI and making other changes until we've resolved any issues with the current python build. We haven't put out a new release yet that has a compatible docker image for the current version, we should do that and iron out any other kinks. Then we can make additional releases with these other PRs that will further change the requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
If you haven't already, please read through our contributing guidelines before opening your PR
What is the purpose of this PR?
Closes #807. Adds support for converting a FOV to an OME-TIFF and vice versa.
How did you implement your changes
Added two functions,
fov_to_ome
andome_to_fov
inload_utils.py
.( moved to tmi)fov_to_ome
: Can convert multiple FOVs with whatever channels necessary to OME-TIFFsome_to_fov
: Can only convert one OME-TIFF to a FOV.Remaining issues