8000 Generalised existing CLAHE implementation to support n dimensions by m-albert · Pull Request #2761 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Generalised existing CLAHE implementation to support n dimensions #2761

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Generalised existing CLAHE implementation to support n dimensions #2761

wants to merge 6 commits into from

Conversation

m-albert
Copy link
Contributor
@m-albert m-albert commented Aug 19, 2017 •
edited
Loading

Description

CLAHE (https://en.wikipedia.org/wiki/Adaptive_histogram_equalization) is well defined for n dimensions and I extended the existing implementation in a straight forward manner.

  • only change in main function equalize_adapthist: handling of default argument
  • changed: _clahe and interpolate naturally extended to nd
  • unchanged: functions clip_histogram and map_histogram

First (and only) tests I performed:

  • using the new equalize_adapthist on a 2d np.uint16 image yields the same result as the old equalize_adapthist
  • new equalize_adapthist yields sensible results in 3d

Checklist

References

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.

@m-albert
Generalised existing CLAHE implementation to support n dimensions:
3566a2e 
  - only change in main function: handling of default argument
  - changed: _clahe and interpolate extended to nd
  - unchanged: functions clip_histogram and map_histogram
@pep8speaks
Copy link
pep8speaks commented Aug 19, 2017 •
edited
Loading

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

Line 149:71: W504 line break after binary operator

Comment last updated at 2019-07-30 17:51:40 UTC

@soupault soupault added this to the 0.14 milestone Aug 19, 2017
@stefanv stefanv requested a review from jni September 12, 2017 00:11
@jni
Copy link
Member
jni commented Sep 14, 2017

@quakberto very cool! Could you add a test that shows this in 3D? Even just checking input against a reference output is useful (to avoid regressions in the future.)

Also, please review your changes to docstrings in terms of whitespace/blank lines, as they violate the NumPy docstring conventions.

Thanks!

@soupault soupault modified the milestones: 0.14, 0.15 May 23, 2018
@soupault soupault modified the milestones: 0.15, 0.16 Apr 20, 2019
@hmaarrfk hmaarrfk mentioned this pull request May 19, 2019
Closed
m-albert added 2 commits July 20, 2019 18:57
@m-albert
- made docstrings compatible with the NumPy docstring conventions (so…
fef4bc2 
…me newlines had disappeared in the previous commit for some reason)

- fixed PEP8 problems in newly added code and also already existing code (mainly split up too long lines)
@m-albert
- added a test to check that equalize_adapthist behaves consistently …
344fb33 
…in Nd: assert that dimensions are conserved from input to output and that the resulting images are similar when constructing a stack of 2d images (using the mean of pixelwise absolute differences)
@m-albert
Copy link
Contributor Author
m-albert commented Jul 20, 2019 •
edited
Loading

Hi, sorry for coming back to this only now. I still think it would be great to have CLAHE for 3d and higher in scikit-image :)

Could you add a test that shows this in 3D? Even just checking input against a reference output is useful (to avoid regressions in the future.)

As you suggested, I added a simple test consisting of two parts:

  1. check that the dimensions are kept in 2d and 3d
  2. check that a plane of the result of a stack of 2d images is similar to the result of the 2d image

Here's a visual comparison (vmin=0, vmax=1 for all images):
image

Also, please review your changes to docstrings in terms of whitespace/blank lines, as they violate the NumPy docstring conventions.

Corrected this, somehow some newlines had disappeared in my first commit.

Two more notes:

  1. As the pull request is pretty old its branch is based on an old master, which is maybe part of the reason the checks fail (despite locally everything being fine)? However it says "Merging can be performed automatically". Would there be a need to rebase? If so, I think I would need some help with it :)

  2. I realised that equalize_adapthist is decorated by @adapt_rgb(hsv_value) to handle color images. In the documentation it says that adapt_rgb is "not recommended for functions that support 3D volumes". So with the current implementation input images with dimension 3 or higher could not be interpreted as color images and 3d images will only be interpreted as such if img.shape[-1]>4. Which would be the right way of handling n-dimensional color images in scikit-image? At the same time, I doubt that many users would want to work with high dimensional color images or 3d grayscale images with very few columns.

Thanks for any comments and let me know what I can improve :)

m-albert added 3 commits July 20, 2019 21:09
@m-albert
- remove expected_warnings in previously added test
9800497
@m-albert
- some more PEP8 corrections
163cd74
@m-albert
Fix two artefacts:
9af2de3 
1) Stripes in starting tiles in each dimension. Solution: Work on integer slices
2) Unprocessed pixels after the last tile in case of the image shape not being a multiple of the kernel size. Solution: Pad the image before processing and crop accordingly before returning
@m-albert m-albert mentioned this pull request Jul 30, 2019
Closed
@m-albert
Copy link
Contributor Author

After adding the proposed fixes to #2219 one of the CLAHE tests fails:
test_adapthist_grayscale

  assert_almost_equal(peak_snr(img, adapted), 102.078, 3)

E AssertionError:
E Arrays are not almost equal to 3 decimals
E ACTUAL: 100.15823498443757
E DESIRED: 102.078

In principle, the existing 2D tests should see these changes:

  • artefacts are taken out
  • different effective tile sizes. Before, the tile sizes could be different from the ones indicated in kernel_size, because of this code:

    scikit-image/skimage/exposure/_adapthist.py

    Lines 119 to 123 in 2ffd9c8

    nr = int(np.ceil(image.shape[0] / kernel_size[0]))
    nc = int(np.ceil(image.shape[1] / kernel_size[1]))
    row_step = int(np.floor(image.shape[0] / nr))
    col_step = int(np.floor(image.shape[1] / nc))

    the new tile sizes should be equal to the kernel_size because the image shape is padded

How to deal with the existing tests?

On another note, one thing that might make sense to add is logic to incorporate the multichannel argument and convert2lab.

@soupault soupault modified the milestones: 0.16, 0.17 Jan 4, 2020
@soupault soupault self-assigned this Jan 4, 2020
@soupault soupault added the ⏩ type: Enhancement Improve existing features label Jan 4, 2020
@rfezzani
Copy link
Member

Hello @m-albert, equalize_adapthist border artifacts have been solved in #4349 without image padding, but the nD support that you address in this PR is still a must!
Do you have by any chance some time to fix this here? Thank you ;-)

@m-albert
Copy link
Contributor Author
m-albert commented Feb 25, 2020 •
edited
Loading

Hi @rfezzani, great that you fixed the border artefacts and thanks for pinging me :)

Do you think it makes sense to continue working with my extension of the existing 2D CLAHE code into nD or directly adapt @anntzer's code (discussed here #2219) which is very elegant and seems to show less artefacts (as you suggested in #4475)?

I'd be happy to work on getting any of the two incorporated into scikit-image.

@rfezzani
Copy link
Member

Thank you @m-albert for your enthusiasm ;-)
I think that the actual implementation should be kept as a base line / fast implementation.
@anntzer version should be then implemented in a separate PR.

@m-albert
Copy link
Contributor Author

@rfezzani That makes sense.

Great, would you then recommend rebasing this (rather old) pull request?

@rfezzani
Copy link
Member

Great, would you then recommend rebasing this (rather old) pull request?

This is up to you ;-) I f you feel more comfortable opening a new PR, just refer to this one and close it.

@RealPolitiX
Copy link
RealPolitiX commented Feb 25, 2020 •
edited
Loading

We (@VincentStimper,@RealPolitiX) have recently implemented an n-dimensional CLAHE algorithm in tensorflow without constraints for the kernel size using padding. We have demonstrated its use with 4D materials science spectroscopy and bioimaging data.
https://github.com/VincentStimper/mclahe
More details on the method can be found in the publication
https://ieeexplore.ieee.org/document/8895993

@rfezzani
Copy link
Member

Wow! @VincentStimper and @RealPolitiX, the demonstrated examples gh-mclahe are really beautiful 👏
It is not planed to add tensorflow as a dependency for skimage, and I refer to this #4478 (comment) concerning the possible integration of mclahe in skimage ;-)

@VincentStimper
Copy link

Thanks!
Since I'm going to rewrite mclahe soon anyway, I could also do a pure numpy implementation if this makes the integration easier.

@pattonw
Copy link
pattonw commented Mar 30, 2020

Hey, I'm also interested in using mclahe. Are there any updates on this topic and is there anything I can do to help?

@soupault soupault removed their assignment Apr 11, 2020
@emmanuelle emmanuelle mentioned this pull request Apr 12, 2020
Closed
@VincentStimper
Copy link

I've finished the NumPy only implementation of MCLAHE (not using Tensorflow), see the numpy branch in my repository
https://github.com/VincentStimper/mclahe/tree/numpy
We could integrate it in skimage now.

@m-albert m-albert mentioned this pull request Apr 20, 2020
Merged
8 tasks
@m-albert
Copy link
Contributor Author

This is up to you ;-) I f you feel more comfortable opening a new PR, just refer to this one and close it.

Was working on this lately and opened a new pull request #4598 with my changes embedded into the current master.

@m-albert
Copy link
Contributor Author
m-albert commented May 1, 2020 •
edited
Loading

Closing this PR in favor of (merged) #4598.

Thank you to everyone involved :)

@m-albert m-albert closed this May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni Awaiting requested review from jni

Assignees
No one assigned
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

equalize_adapthist leaves artefacts when image size is not a multiple of tile size
8 participants
@m-albert @pep8speaks @jni @rfezzani @RealPolitiX @VincentStimper @pattonw @soupault
0