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.

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

restoration.denoise_bilateral and negative value image #1679

Closed
wants to merge 10 commits into from
Closed

restoration.denoise_bilateral and negative value image #1679

wants to merge 10 commits into from

Conversation

odebeir
Copy link
Contributor

@odebeir odebeir commented Aug 29, 2015 •
edited by sciunto

Here is a fix for #1677 regarding the case of negative-valued images, not fixed in #2533
basically change the test inside .pyx and adjusted docstring

@jni
Copy link
Member

jni commented Aug 29, 2015

@odebeir, @emmanuelle mentions that we could just offset the image values and back if there are any negative values. This seems easy enough to do? What do you think?

emmanuelle
emmanuelle reviewed Aug 29, 2015
View reviewed changes
skimage/restoration/_denoise.py Outdated
@@ -23,7 +23,7 @@ def denoise_bilateral(image, win_size=5, sigma_range=None, sigma_spatial=1,
Parameters
----------
image : ndarray
Input image.
Input image (image minimum must be >0).
Copy link
Member

Choose a reason for hiding this comment

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

It's not completely true: values must be >= 0, it's the maximum that has to be >0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact is one value is <0 it breaks as well

@emmanuelle
Copy link
Member

What I suggest is

  • substract the minimum (and add it again before returning the output, of course)
  • when testing whether the maximum is zero, don't raise an error but return the (constant) input if zero
  • add a test with an array of zeros and an array of negative values in the test module

@emmanuelle
Copy link
Member

@jni would you agree?

ahojnnes
ahojnnes reviewed Aug 29, 2015
View reviewed changes
skimage/restoration/_denoise.py Outdated
@@ -23,7 +23,7 @@ def denoise_bilateral(image, win_size=5, sigma_range=None, sigma_spatial=1,
Parameters
----------
image : ndarray
Input image.
Input image (image must be >= 0 with at least one > 0 value).
Copy link
Member

Choose a reason for hiding this comment

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

How about: "Input image whose minimum value is greater than 0."

@odebeir
Copy link
Contributor Author

odebeir commented Aug 30, 2015

I added three tests for the function:

  • check if a zero array returns identity
  • check if a const negative image returns identity
  • check if 2 images with an offset (eventually negative pixels) give the same result (with the same offset)

the modified python wrapper _denoise pass these tests

@odebeir
Copy link
Contributor Author

odebeir commented Sep 3, 2015

it is amaizing:
adding test actually diminishes the coverage ...

emmanuelle
emmanuelle reviewed Sep 3, 2015
View reviewed changes
skimage/restoration/_denoise.py Outdated
# if all value identical, returns the same image
return image
else:
return _denoise_bilateral(offset_img+1, win_size, sigma_range, sigma_spatial,
Copy link
Member

Choose a reason for hiding this comment

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

why the +1 and -1? I don't understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it was related to a previous correction (commit 8d6ba04),
now, with the offset in the python wrapper it is indeed not needed anymore, so I reverted the .pyx to its initial state and removed the +1/-1

@emmanuelle
Copy link
Member

yeah, coveralls is doing strange things these days. Nevermind, your new tests are indeed useful. I just added a couple of comments.

ahojnnes
ahojnnes reviewed Sep 4, 2015
View reviewed changes
skimage/restoration/_denoise.py
# if all value identical, returns the same image
return image
else:
return _denoise_bilateral(offset_img, win_size, sigma_range, sigma_spatial,
Copy link
Member

Choose a reason for hiding this comment

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

This is changing the existing behavior and I don't see a reason why to subtract the min from the image. If you want to test for uniqueness, do a image.min() - image.max() == 0 test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically the idea was to enable the process on image having <0 values...
using the offset ensures it as suggested by @emmanuelle

Copy link
Member

Choose a reason for hiding this comment

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

I agree that negative values should be supported, but we shouldn't mess with the range of values in the image. This should be easily possible by changing the Cython implementation accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so, the solution was only a (quick) fix.

@stefanv
Copy link
Member

stefanv commented Dec 11, 2015

@ahojnnes @odebeir What is the next step forward on this one?

@soupault soupault added the bug label Jan 30, 2016
@soupault soupault changed the title Fix1677 restoration.denoise_bilateral and zeros image Feb 4, 2016
@soupault soupault mentioned this pull request Feb 22, 2017
Merged
@jni
Copy link
Member

jni commented Feb 22, 2017

Oops, this one fell through the cracks... @odebeir do you think you could rebase on the current master? Then we can, hopefully, do a quick final review and get it merged!

@odebeir
Copy link
Contributor Author

odebeir commented Feb 22, 2017

@jni, I am a bit confused here, I am not sure to be able to do the suggested maneuver ...

@soupault
Copy link
Member

soupault commented Feb 26, 2017 •
edited

deleted

@soupault soupault closed this Feb 26, 2017
@jni
Copy link
Member

jni commented Feb 26, 2017

@soupault No, that fixed the case of single-value images. This fixes the case of negative-valued images.

@jni jni reopened this Feb 26, 2017
@sciunto sciunto closed this Aug 30, 2018
@sciunto sciunto reopened this Aug 30, 2018
@pep8speaks
Copy link

Hello @odebeir! Thanks for updating the PR.

Line 66:80: E501 line too long (83 > 79 characters)
Line 67:31: E128 continuation line under-indented for visual indent

@sciunto sciunto changed the title restoration.denoise_bilateral and zeros image restoration.denoise_bilateral and negative value image Aug 30, 2018
@sciunto
Copy link
Member

sciunto commented Aug 30, 2018

I updated the first message and the title to avoid confusion

@rfezzani rfezzani added 🩹 type: Bug fix Fixes unexpected or incorrect behavior and removed type: bug labels Feb 22, 2020
Base automatically changed from master to main February 18, 2021 18:22
@mkcor mkcor mentioned this pull request Feb 22, 2021
Closed
@grlee77 grlee77 mentioned this pull request Aug 22, 2021
Merged
@grlee77
Copy link
Contributor

grlee77 commented Aug 22, 2021

This PR seems like a useful, straightforward addition that never quite got merged. I fixed the merge conflicts and have revived it in #5527

@grlee77 grlee77 closed this Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@odebeir @jni @emmanuelle @stefanv @soupault @pep8speaks @sciunto @grlee77 @ahojnnes @rfezzani