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

fix-2551: prevented NaN in convoluted matrix #2553

Closed
wants to merge 1 commit into from
Closed

fix-2551: prevented NaN in convoluted matrix #2553

wants to merge 1 commit into from

Conversation

meriki
Copy link

@meriki meriki commented Mar 5, 2017

Description

[Tell us about your new feature, improvement, or fix! If relevant, please supplement the PR with images.]

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

@meriki meriki mentioned this pull request Mar 5, 2017
4 tasks
@codecov-io
Copy link

Codecov Report

Merging #2553 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2553      +/-   ##
==========================================
+ Coverage   90.53%   90.53%   +<.01%     
==========================================
  Files         304      304              
  Lines       21706    21707       +1     
  Branches     1872     1872              
==========================================
+ Hits        19652    19653       +1     
  Misses       1714     1714              
  Partials      340      340
Impacted Files Coverage Δ
skimage/restoration/deconvolution.py 93.1% <100%> (+0.08%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b692ea7...4ff813a. Read the comment docs.

@soupault
Copy link
Member

soupault commented Mar 6, 2017

@meriki you need to justify this change by adding a test which will fail on old code and work fine on yours. Also, ensure that your code is PEP-8 compliant (as it mentioned in the PR template).

@jni
Copy link
Member

jni commented Mar 7, 2017

@meriki to elaborate on @soupault's point, please look at the tests in skimage/restoration/tests/test_restoration.py. Add a function test_flat_image there that would have resulted in NaN's in the old code but is fixed in the new code. Thanks!

@@ -387,6 +387,7 @@ def richardson_lucy(image, psf, iterations=50, clip=True):

for _ in range(iterations):
relative_blur = image / convolve_method(im_deconv, psf, 'same')
relative_blur[np.isnan(relative_blur)] = 0 # to prevent NaN from propogating throughout convulated matrix
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Propagating.

And, "result of convolution" instead of "convulated matrix"?

@stefanv stefanv closed this Feb 18, 2021
@stefanv stefanv deleted the branch scikit-image:master February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants