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 colocalization metrics #6189

Merged
merged 43 commits into from Jan 23, 2023

Conversation

thanushipeiris
Copy link
Contributor

@thanushipeiris thanushipeiris commented Jan 18, 2022

Description

This pull request implements several commonly used pixel-based colocalization metrics for fluorescent microscopy:

  1. Pearson's Correlation Coefficient: pearson's correlation of pixel intensities across two matching channels
  2. Manders' Colocalization Coefficient paper
  3. Manders' Overlap Coefficient paper
  4. Intersection Coefficients: fraction of a segmented channel that overlaps with another segmented channel
  5. Pixel intensity sum: sum of pixel values within segmentation mask of a channel
  6. Average pixel intensity: average pixel value within segmentation mask of a channel

A roi parameter is provided for each function for users to limit analysis to a particular region of interest.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@pep8speaks
Copy link

pep8speaks commented Jan 18, 2022

Hello @thanushipeiris! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 10:80: E501 line too long (81 > 79 characters)
Line 24:80: E501 line too long (192 > 79 characters)
Line 38:37: E231 missing whitespace after ','
Line 38:39: E231 missing whitespace after ','
Line 38:41: E231 missing whitespace after ','
Line 51:50: E226 missing whitespace around arithmetic operator
Line 53:53: E226 missing whitespace around arithmetic operator
Line 53:71: E231 missing whitespace after ','
Line 65:80: E501 line too long (91 > 79 characters)
Line 68:80: E501 line too long (95 > 79 characters)
Line 71:80: E501 line too long (91 > 79 characters)
Line 75:80: E501 line too long (95 > 79 characters)
Line 99:80: E501 line too long (80 > 79 characters)
Line 102:80: E501 line too long (80 > 79 characters)
Line 111:80: E501 line too long (80 > 79 characters)
Line 112:80: E501 line too long (80 > 79 characters)
Line 136:80: E501 line too long (88 > 79 characters)
Line 139:80: E501 line too long (85 > 79 characters)
Line 161:80: E501 line too long (80 > 79 characters)

Line 813:1: E302 expected 2 blank lines, found 1

Comment last updated at 2022-06-24 05:00:40 UTC

skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/__init__.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @thanushipeiris. Most of these seem reasonable to provide, but I think perhaps the sum and average cases are a bit too simple and we can let users just do those operations on their own.

I have left some initial comments, but this will need another round of review later. I did not yet look at the tests that were added.

skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks for making the initial updates. See some minor suggestions and a couple of questions about the valid image range for the Manders measures.

skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/_shared/utils.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/_colocalization.py Outdated Show resolved Hide resolved
skimage/measure/tests/test_colocalization.py Outdated Show resolved Hide resolved
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates @thanushipeiris. I am fine with the implementation, but we could use an example somewhere under doc/examples if you can think of a good demo case using these.

For example, for Pearson I would expect the correlation to be high for color channels of a typical RGB image like astronaut, but low for the two channels of cells3d (where one channel is nuclei and the other is membranes). Do we have a reasonable dataset to show typical usage for these Manders coefficients?

@grlee77 grlee77 added the ⏩ type: Enhancement Improve existing features label Apr 1, 2022
@grlee77
Copy link
Contributor

grlee77 commented Apr 1, 2022

Hi @thanushipeiris, just checking in here. I added a small docstring formatting fix. Any chance you could add an example for the gallery?

@thanushipeiris
Copy link
Contributor Author

Yep will do in the next couple of days, if I find a good set of images with colocalized stains

@alexdesiqueira
Copy link
Member

@thanushipeiris thank you for all your work so far, it looks great! Would you like to add an example on the gallery? If you don't have time available, we could continue this PR from here. Thanks again!

@mkcor
Copy link
Member

mkcor commented Jan 21, 2023

Is the addition of .hypothesis/unicode_data/13.0.0/charmap.json.gz intentional? 🤔

Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Just a small comment, this PR removes the executable permissions on dev.py. Probably useful to keep this. Did you do this intentionally? I think I saw a similar change elsewhere and wondering if git or some other tool is messing with us.

@thanushipeiris
Copy link
Contributor Author

thanushipeiris commented Jan 22, 2023

Is the addition of .hypothesis/unicode_data/13.0.0/charmap.json.gz intentional? 🤔

Nope, not sure where this came from and I have deleted it.

Just a small comment, this PR removes the executable permissions on dev.py. Probably useful to keep this. Did you do this intentionally? I think I saw a similar change elsewhere and wondering if git or some other tool is messing with us.

As far as I can see, this PR's dev.py is identical to the scikit-image main branch, even though it's showing up as an "empty" file in the Files changed section of this PR.

@thanushipeiris
Copy link
Contributor Author

Also the tests are failing with ModuleNotFoundError: No module named 'skimage.measure._colocalization' even though the file skimage.measure._colocalization.py very clearly exists.

This has been happening for a long time on this PR and I'm not sure why or how to fix it...

@lagru
Copy link
Member

lagru commented Jan 22, 2023

it's showing up as an "empty" file in the Files changed section of this PR.

I think GitHub's GUI is just confusing in this instance and the "Empty file." bit just means that there is no diff to show. If you look behind the name of the file here you can see GitHub showing "100755 → 100644"; I guess that's why it shows up in this PR at all. Maybe a leftover from the merge conflict as well.

Are you on Linux or similar? Then you can revert that bit with chmod +x dev.py. But I'm also happy to do it for you if you are on Windows. 🙂

@lagru
Copy link
Member

lagru commented Jan 22, 2023

Also the tests are failing with ModuleNotFoundError: No module named 'skimage.measure._colocalization' even though the file skimage.measure._colocalization.py very clearly exists.

We are switching to a new meson-based build system. The error should be resolved if you add the file _colocalization.py to the meson.build in the same directory.

Oh, and you'll likely need to do the same for test_colocalization.py but in the test directory of course.

@jarrodmillman jarrodmillman changed the title added colocalization metrics Add colocalization metrics Jan 22, 2023
@jarrodmillman
Copy link
Contributor

@lagru This looks good to me. I am thinking about releasing 0.20.0rc0 tomorrow and wanted to check whether you thought we should merge this before or after 0.20. If before, feel free to merge.

"is_thing" implies that only a check is performed. However, as this
function also performs a cast at the end, "as_thing" is likely a
clearer name.

Also be more explicit about the signature.
plt.show() isn't necessary to show the last plot.
@lagru
Copy link
Member

lagru commented Jan 23, 2023

I took the liberty to push a few last minor fixes and suggestions. I'll merge once the CI is green. Thank you @thanushipeiris and everyone else who was involved!

@lagru lagru merged commit 690488d into scikit-image:main Jan 23, 2023
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants