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

Add conversion support for index images to ImgLib2 and legacy ImageJ ROIs #283

Merged
merged 13 commits into from
Jan 9, 2024

Conversation

elevans
Copy link
Member

@elevans elevans commented Aug 28, 2023

This PR adds support to convert index images (Python or Java) into either of the following:

  1. A net.imagej.roi.DefaultROITree populated with ImgLib2 ROIs.
  2. ImageJ RoiManager populated with legacy converted ROIs (e.g. net.imagej.legacy.convert.roi.polygon2d.Polygon2DWrapper).

Because there's no real way to tell the difference between an index image and some other type of image data (without making assumptions) I feel its best to not register these index image functions with scyjava/pyimagej converters.

Included in this PR:

  • converters for index images into ROIs
  • tests for index images to ROIs
  • Cellpose/StarDist use case

To run the use case you'll need Cellpose and StarDist. This environment.yml file should do the trick:

name: cellstar
channels:
    - conda-forge
    - defaults
dependencies:
    - pyimagej
    - jupyterlab
    - openjdk=11
    - pip
    - pip:
        - tensorflow==2.12.*
        - stardist
        - cellpose

Don't forget to pull this branch and pip install -e . before running the notebook!

@elevans elevans requested review from hinerm and gselzer August 28, 2023 15:25
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (c003095) 77.68% compared to head (f205852) 77.81%.

Files Patch % Lines
src/imagej/convert.py 64.00% 9 Missing ⚠️
src/imagej/_java.py 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
+ Coverage   77.68%   77.81%   +0.13%     
==========================================
  Files          16       16              
  Lines        1949     2010      +61     
==========================================
+ Hits         1514     1564      +50     
- Misses        435      446      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

Great tutorial @elevans!

Just had some small suggestions.

doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Show resolved Hide resolved
src/imagej/convert.py Show resolved Hide resolved
src/imagej/convert.py Outdated Show resolved Hide resolved
src/imagej/convert.py Outdated Show resolved Hide resolved
@hinerm
Copy link
Member

hinerm commented Aug 30, 2023

@elevans I created an environment with the indicated yml and pip install -e . but when I run the notebook the first cell fails with a dependency not found exception on StarDist.

When I paste the exact same line into a python interpreter launched in the exact same environment that started jupyter, it works no problem. :(

I noticed that I'm using python 3.8 locally and I think you used 3.11 when you made the ipynb. Is 3.11 necessary?

@gselzer
Copy link
Contributor

gselzer commented Aug 30, 2023

@hinerm I ran using the following environment.yml:

name: cellstar
channels:
    - conda-forge
    - defaults
dependencies:
    - python=="3.8"
    - pyimagej
    - jupyterlab
    - openjdk=11
    - pip
    - pip:
        - tensorflow==2.12.*
        - stardist
        - cellpose

and things seem to work fine. How did you install python 3.8?

@hinerm
Copy link
Member

hinerm commented Aug 30, 2023

How did you install python 3.8?

@gselzer it's just my system python, since it wasn't specified in the environment.yml specified.

I'm trying with your method of specification. Interestingly I earlier tried >=3.11 and environment creation failed with dependency conflicts.

Further interestingly, python=="3.8" syntax does not work on my computer. I have to remove the double quotes.

Edit: specifying python==3.8 did not work either. Still can't find StarDist

image

@elevans
Copy link
Member Author

elevans commented Oct 11, 2023

@hinerm I updated the jupyter notebook to use %pip install package magic per Curtis's recommendation. Would you mind giving the notebook a try again?

@hinerm
Copy link
Member

hinerm commented Oct 13, 2023

@hinerm I updated the jupyter notebook to use %pip install package magic per Curtis's recommendation. Would you mind giving the notebook a try again?

Do I need to do anything different to install it? I assume if you use %pip install you need to trust the notebook?

I assume the environment file should now be:

name: cellstar
channels:
    - conda-forge
    - defaults
dependencies:
    - python=3.8
    - pyimagej
    - jupyterlab
    - openjdk=11
    - pip

Anyway, this doesn't work for me. It looks like the stardist has a dependency on csbdeep that is not compatible with tensorflow 2.12.

What version of stardist and csbdeep are you getting from pip? I think we want to pin a stardist version compatible with the chosen tensorflow on all OS's. Welcome to dependency hell.

image

elevans added 10 commits January 3, 2024 14:19
This commit adds converts that convert index images
(java or python) into ImgLib2 rois (in a ROITree) or
as ImageJ rois populated in the RoiManager.
If the current session is in headless mode
catch the JException and do nothing.
I added a better description and more comments
to the code. I also fixed a typo.
Instead of using NumPy arrays to generate boolean
masks for each label (an expensive operation), we can use
an ImgLabeling and query the regions instead. This method
is 5.73 times faster than the my previous NumPy implementation.
After troubleshooting on Linux, Windows and macOS I found
using a combination of mamba and pip commands worked best.
I also made some minor changes for macOS. If macOs is
detected, the notebook changes its mode from `interactive`
to `headless`.
@elevans
Copy link
Member Author

elevans commented Jan 3, 2024

@hinerm After many hours of painful testing across Linux, Windows and macOS I found the following shell commands to be the most successful for installing cellpose and stardist:

%%capture
!mamba install -y -c conda-forge tensorflow stardist
!pip install cellpose

I tested this on the following systems:

  • Windows 10/11
  • Linux (Ubuntu 22.04.3 LTS)
  • macOS Sonoma

While testing on macOS, I got bit by the lack of interactive mode support. Bummer! So I included a check for macOS and switch to headless mode with some explanations.

When displaying the final image output the cytoplasmic
channel is too dim to see. The user needs to adjust the
brightness/contrast in ImageJ/Fiji.
@elevans
Copy link
Member Author

elevans commented Jan 8, 2024

I'm going to do something spicy 🌶️ and merge this branch. The actual new code in convert.py and the accompanying tests have already been reviewed since @hinerm had troubles with the Jupyter notebook. Letting mamba/conda resolve the tensorflow and stardist side of things while installing cellpose from PyPi and not conda-forge was the trick. Since then all I've done is update the cellpose jupyter notebook with shell commands, compatibility with macOS and extra notes.

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Thank you @elevans—this is great work. I have a couple of requests, though:

  • I'd like to avoid promising DefaultWritablePolygon2D, in favor of using the interface where needed.
  • Ideally, the conversion logic between index images and ImgLib2 ROIs would live in imagej-common, and simply get called from Python. Then people on the Java side can take advantage of it as well in their scripts.

I would be OK merging this branch once the issue with DefaultWritablePolygon2D is addressed, since we can add the Java-side conversion logic afterward and then replace the code here with a more concise method call. But I would appreciate it if you could do it soon, before we forget about it.

Finally, about the brightness/contrast adjustment: did you try automating that? If so, did you run into any problems? I'm happy to help figure out how to do it, and/or fix any bugs relating to that, as needed.

src/imagej/convert.py Outdated Show resolved Hide resolved
src/imagej/convert.py Show resolved Hide resolved
src/imagej/convert.py Outdated Show resolved Hide resolved
doc/Cellpose-StarDist-Segmentation.ipynb Outdated Show resolved Hide resolved
Instead of using the default implementation,
DefaultWritablePolygon2D, I should use the
interface: WritablePolygon2D.
Apply the Brightness/Contrast standard "auto"
adjust to the second channel (cytoplasmic channel) after
displaying the data.
@ctrueden ctrueden merged commit a173d5a into main Jan 9, 2024
10 checks passed
@ctrueden ctrueden deleted the index-img-to-roi branch January 9, 2024 03:50
@ctrueden
Copy link
Member

ctrueden commented Jan 9, 2024

Awesome, thanks @elevans!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants