The Wayback Machine - https://web.archive.org/web/20201010185841/https://github.com/scikit-image/scikit-image/pull/4828
Skip to content

/ scikit-image

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

Jump to bottom

Fix equalize_adapthist (CLAHE) for clip value 1.0, fix docstrings #4828

Open
wants to merge 4 commits into
base: master
from
Open

Fix equalize_adapthist (CLAHE) for clip value 1.0, fix docstrings #4828

wants to merge 4 commits into from

Conversation

@funkey
Copy link

@funkey funkey commented Jul 7, 2020

Description

Addresses #4827 and fixes docstrings.

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.
funkey added 3 commits Jul 7, 2020
@funkey
Fix equalize_adapthist (CLAHE) for clip value 1.0
a9588fe 
A clip value of 1.0 is equal to not applying histogram clipping at all.
When passed to equalize_adapthist (CLAHE), the expected behavior is
therefore to perform AHE (i.e., CLAHE without clipping). The previous
implementation, however, returned the input image.
@funkey
Use kernel_size instead of NR_OF_GRAY for largest clip value
be97eac 
The largest value in a histogram bin is determined by the kernel size,
not by the number of gray levels.
@funkey
Fix docstring of _clahe
Loading status checks…
4b16421 
The docstring said:

> A clip limit smaller than 1 results in standard (non-contrast limited)
> AHE.

but limits of 0 or larger equal 1 result in AHE. Doc string corrected
accordingly.

Furthermore the docstring said:

> The output image will have the same minimum and maximum value as the
> input image.

which is neither true for CLAHE nor for AHE. Removed sentence.
@funkey
Copy link
Author

@funkey funkey commented Jul 7, 2020

This test fails now, which was authored by @rfezzani and @sciunto. Can you confirm that this is really the expected behavior?
@sciunto
Copy link
Member

@sciunto sciunto commented Jul 7, 2020

Why this test wouldn't be correct in your opinion?
@funkey
Copy link
Author

@funkey funkey commented Jul 7, 2020 •
edited

Hi François,

I mentioned that briefly in #4827, but let me provide some more details here of how I understand the clip_limit parameter: From the docstring and the implementation, I assume that clip_limit is relative to the size of the kernel (i.e., the effective, absolute clip limit is clip_limit * np.prod(kernel_size)). A clip limit of 0.5, for example, would limit the values of the histogram of intensities to be at most 50% of all pixels in the kernel. Excess values in the histogram are evenly distributed across the other bins, hence performing the contrast limiting (CL) part of CLAHE. A clip limit of 1.0, as I understand it, would effectively disable clipping (no bin in the histogram can have more than 100% of all pixels). However, that doesn't mean that the input image remains unchanged. Adaptive histogram equalization is still performed (i.e., AHE), replacing the intensity of each pixel with its rank divided by kernel size.

This understanding is also in line with the progression of histogram equalization over clip limits close to 1.0. Compare, for instance, clip values of 0.9, 0.95, 1.0, and 1.05. The last value performs AHE in the current implementation. The first two values perform CLAHE with decreasing effect of clipping, i.e., in the limit AHE. In this sequence, I would find it surprising if 1.0 would not perform AHE, but instead return the original image.
@sciunto
Copy link
Member

@sciunto sciunto commented Jul 7, 2020

I'm afraid, as far as I remember, that we implemented the test after reading the misbehavior. It seems to me that you are correct, any other opinion @scikit-image/core ?
@alexdesiqueira alexdesiqueira mentioned this pull request Jul 14, 2020
Open
@mkcor
mkcor reviewed Jul 17, 2020
View changes
skimage/exposure/_adapthist.py
@@ -119,8 +117,7 @@ def _clahe(image, kernel_size, clip_limit, nbins):
The number of "effective" graylevels in the output image is set by `nbins`;
selecting a small value (eg. 128) speeds up processing and still produce

This comment has been minimized.

Sign in to view
@mkcor

mkcor Jul 17, 2020
Contributor
Suggested change
selecting a small value (eg. 128) speeds up processing and still produce
selecting a small value (e.g. 128) speeds up processing and still produces

@mkcor
mkcor reviewed Jul 17, 2020
View changes
skimage/exposure/_adapthist.py
@@ -119,8 +117,7 @@ def _clahe(image, kernel_size, clip_limit, nbins):
The number of "effective" graylevels in the output image is set by `nbins`;
selecting a small value (eg. 128) speeds up processing and still produce
an output image of good quality. The output image will have the same
minimum and maximum value as the input image. A clip limit smaller than 1
an output image of good quality. A clip limit of 0 or larger equal 1

This comment has been minimized.

Sign in to view
@mkcor

mkcor Jul 17, 2020
Contributor

Is "a clip limit either == 0 or >=1" what is intended?

This comment has been minimized.

Sign in to view
@funkey

funkey Jul 28, 2020
Author

For CLAHE, the clip value would be in the open interval (0, 1). Clip values >=1 essentially disable clipping and therefore simply perform AHE. A clip value of 0 does not really make sense, so the implementation falls back to AHE in this case.

@jni
Copy link
Contributor

@jni jni commented Jul 31, 2020

From a cursory glance, I agree with these changes and that the test should be updated. @funkey (👋) can you update the test accordingly?
@funkey
Fix tests for equalize_adapthist with clip_limit in {0, 1}
Loading status checks…
b8b68f2
@sciunto sciunto added this to the 0.18 milestone Aug 26, 2020
@funkey
Copy link
Author

@funkey funkey commented Aug 26, 2020

I believe the tests are fixed, the CircleCI error seems unrelated to me. Let me know if I can do anything else!
@stefanv
Copy link
Member

@stefanv stefanv commented Aug 27, 2020

Thanks for investigating, @funkey!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkcor mkcor

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
0.18
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@funkey @sciunto @jni @stefanv @mkcor @alexdesiqueira
You can’t perform that action at this time.