-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 image enhancement measure (EME) #3776
base: main
Are you sure you want to change the base?
Conversation
Hello @iliailmer! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-11-11 18:26:51 UTC |
Hi @iliailmer, Thank you for the contribution! I have not yet done a detailed review of the publication, but it has a substantial number of citations and the implementation looks simple and should be easy to maintain going forward. I think this is a reasonable candidate for inclusion in scikit-image. I can do a more thorough review over the coming week, but a few initial comments: 1.) The function should not be stored in the top-level 2.) Use a more descriptive name for the function than 3.) It looks like this can be trivially extended to the n-dimensional case rather than just 2d and 3d. We can help with this if you have questions on how to approach it. A convention in the library is to include a |
…re/simple_metrics.py, added footprint parameter for n-dimensional filtering
Hi @grlee77 , thanks for your comments! Regarding the multidimensional extension, I realize now that the max/min filters used here are, by default, usable with any number of dimensions, so one does not need to check the length of the array. In this case the |
Looks promising! I'll have a more thorough look later. |
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.
Thanks for addressing @grlee77's feedback. Good work so far! With the recent changes this should work for the n-dimensional case, perhaps we could mention that in the docstring. I think you forgot to remove the now obsolete file image_quality_measure
. Could you fix that?
Otherwise I don't yet fully understand your implementation when looking at the linked paper (see comment). Do you or someone else know a source for the clock image used in the paper? It would be useful to compare results of this function to the paper.
I'm getting confused now. The diff on GitHub shows that this PR still includes 2 redundant files with nearly the same code. Could you ensure that the redundant files are removed? We only want to keep the changes in |
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.
@iliailmer thanks for addressing my feedback. The solution with eps
as an optional parameter is much better now but I'm not sure whether this is the right way to solve this. As I understand the paper the formula is meant to quantify a contrast improvement somehow. While rescaling the image won't affect the Weber contrast I'm unsure about this measurement.
Is there a reason to rescale between [0, 1] in particular? If we were to chose a value above zero as the lower bound we could omit the eps
parameter altogether.
Hi @iliailmer thanks for your PR. It looks good, but we're missing unit tests (write unit tests on small images, 2D and 3D, with int and float types for example) as well as a gallery example. |
@emmanuelle Thank you, I will get to it as soon as I can. Should the test images come from the |
@iliailmer yes, either images from |
I keep getting Tried to fix docstring couple of times already, not sure where the unindent is happening |
Have you tried the solution proposed here? |
@rfezzani Thank you, it worked. |
skimage/measure/simple_metrics.py
Outdated
"A new measure of image enhancement.", | ||
IASTED International Conference on Signal Processing | ||
& Communication, Citeseer, 2000, | ||
:DOI:10.1.1.35.4021 |
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.
:DOI:10.1.1.35.4021 | |
:DOI:`10.1.1.35.4021` |
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.
The DOI is incorrect. Also, backquotes must be set as suggested.
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.
Hi @sciunto, thanks for pointing that our!
I was having troubles with the DOI though.
The only source that lists the DOI is this link, and I doubt that there is a better version or at least i couldn't find one. The doi.org says this is only a prefix, but google search for doi=10.1.1.35.4021 returns the link above no problem. The original journal is not available (see here) so I am not sure what is the best way to prcoceed here.
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.
I think doi.org must be the reference. If no working doi can be found, it is better to do not display a doi. Eventually, a link to the pdf file can be added instead.
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.
I removed it for now.
Hi @stefanv, could you please take a look and check if anything is amiss? All the checks are passed so far. Thanks! |
Hey @iliailmer, sorry for the slow review process. I still want to get this into scikit-image! There are some merge conflicts now, do you want to resolve these or shall I? Afterwards I can do another review and hopefully approve to get this going again. 🙏 |
Hey, I took the liberty to merge this which was a little complicated due to us moving |
simple_metrics was moved from skimage/measure to skimage/metrics making it quite complicated to preserve the changes during the merge. This commit re-adds the original changes authored by iliailmer <i.ilmer@icloud.com> back. Co-authored-by: iliailmer <i.ilmer@icloud.com>
Type hints are not (yet) used within scikit-image. Within the docstring I tried to describe the measure in more precise terms. However, the paper seems a bit vague on the interpretation of this measure.
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.
I took the liberty and tried to make the docstring use a more precise description of the EME. However, I'm still a bit vague on how to EME should be interpreted precisely and what kind of "enhancement" it measures. Does it purely measure contrast enhancement? To me that seems to me where we can gain the most value yet.
Otherwise I suggested some smaller but easier changes that should probably be addressed.
>>> before = enhancement_measure(img) | ||
>>> after = enhancement_measure(equalize_hist(img)) | ||
>>> before < after | ||
True |
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.
Other measure functions in this module are applied on and compare two images. As far as I understand, the EME of an image is not useful on its own but rather it's difference to the EME of an improved image. It might be worth thinking about making the function take two images and return a bool or difference between the EME of both images. That way we also prevent users from assigning to much meaning to the absolute value of the measure.
estimate of image quality, useful when comparing image enhancement algorithms. | ||
|
||
""" | ||
import numpy as np |
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.
import numpy as np |
Not used.
# We will create a function that takes an original image as `before` | ||
# and applies a `transform` to it. We will use a gaussian filter to |
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.
Nitpick: the function doesn't actually apply a transform to it. It just calls enhancement_measure
on both and plots the results. Rewording this might be a lot clearer to new users.
---------- | ||
image : array | ||
Input image of which the quality should be assessed. | ||
Can be either 3-channel RGB or 1-channel grayscale. |
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.
We should probably support only grayscale images as input to make sure that the user determines how the total luminance of a colored image is calculated. Currently, the way that maximum_filter
and minimum_filter
is combining color channels is not something that is covered by the paper.
@@ -259,3 +263,51 @@ def normalized_mutual_information(image0, image1, *, bins=100): | |||
H01 = entropy(np.reshape(hist, -1)) | |||
|
|||
return (H0 + H1) / H01 | |||
|
|||
|
|||
def enhancement_measure(image, size=3, eps=1e-6): |
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.
Is there some reasoning between choosing eps=1e-6
as the default? Is something else more reasonable?
|
||
|
||
def compare_side_by_side(before, after): | ||
fig, ax = plt.subplots(nrows=1, ncols=2, figsize=(16, 14)) |
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.
fig, ax = plt.subplots(nrows=1, ncols=2, figsize=(16, 14)) | |
fig, ax = plt.subplots(nrows=1, ncols=2) |
I think removing the forced figure size leads to a nicer layout. Otherwise the images seem very small.
assert_less(enhancement_measure(orig, size=5), | ||
enhancement_measure(enhanced, size=5)) | ||
assert_less(enhancement_measure(orig, size=11), | ||
enhancement_measure(enhanced, size=11)) |
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.
We should probably check the actual values of the EMEs and that they are within an expected range. That way our tests would inform us if some change leads to significantly different EMEs.
Hey @iliailmer, would you be able to look into the feedback? This seems like something that would be cool to have in scikit-image. It's also totally fine if you don't have the bandwidth in which case maybe someone else could take over. :) |
Description
This PR contains implementation of an image quality measure called EME, based on this paper.
It is based no the human visual system, as mentioned in the paper, and it can be used as a metric for enhancement algorithms to see if they work or not.
Checklist
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.@meeseeksdev backport to v0.14.x