iradon filter function #3099
iradon filter function #3099
Conversation
…B and Octave iradon functions.
|
Hello @kczimm! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-08-30 21:25:56 UTC |
Codecov Report
@@ Coverage Diff @@
## master #3099 +/- ##
=========================================
Coverage ? 85.94%
=========================================
Files ? 336
Lines ? 27340
Branches ? 0
=========================================
Hits ? 23498
Misses ? 3842
Partials ? 0
Continue to review full report at Codecov.
|
| pass | ||
| elif filter_name == "shepp-logan": | ||
| # Start from first element to avoid divide by zero | ||
| kernel[1:] *= np.sin(omega[1:] / 2) / (omega[1:] / 2) |
There was a problem hiding this comment.
Should be sinc.
Since sinc is defined as sin(pi x) / (pi x), I suggest using f instead of omega. and doing
f = fftfreq(size)
kernel *= np.sinc(f)
| n2 = np.arange(size / 2 - 1, 0, -1, dtype=np.int) | ||
| n = np.concatenate((n1, n2)) | ||
| f = np.zeros(size) | ||
| f[0] = 0.25 |
There was a problem hiding this comment.
please mention Kak & Slaney here and the number of the equation, otherwise it is very difficult to understand where it comes from (as opposed to the naive ramp filter, which is much easier to understand)
|
Thanks, LGTM. With your improvements, is it possible to decrease the tolerance in the |
|
I suggest that we merge your PR first, and then see if we must also include changes from #2165. |
|
@emmanuelle I created that tolerance to have a passing test. There is still some bias in the reconstruction but with this PR, it is significantly reduced. I cannot decrease the tolerance by an order of magnitude without the test failing. However, the error is small enough that after a user converts to Hounsfield units (which is a common use case in CT applications) the error will still be less than 0.1 HU bias using the ramp filter. |
|
This is so close with only minor modifications needed before merge. @kczimm would you like to take a look, or should @scikit-image/core polish it up before merging? |
|
@stefanv if @scikit-image/core can polish it up, I'm OK with that. This was so long ago that I did this that it might take me more effort to get back up to speed than the core team would need to finish it off. If it won't go through otherwise, I'll finish it up. It shouldn't stay in scikit-image broken. |
|
@kczimm I apologize for letting this issue slide; thank you again for your work. |
d8e24a3 to
0bdef5c
Compare
|
Because digging this one out took alot of merging, I decided to revert my changes to this PR (sorry for the force push), and move them to: #4950 so we can visually compare. I think there were two goals for this PR:
Therefore, the main contribution here is the refactoring of the iradon_filters function. I now need time to review this, |
|
@kczimm, the |
|
I think this was quite an elegant refactoring, and surprisingly it has aged well. @kczimm is there any chance you want to pick it back up? |
Implementation by @kczimm from scikit-image#3099 Co-authored-by: kczimm <kevinczimmerman@gmail.com>
|
See #6873 |
|
@stefanv thanks for doing this and sorry I'm just seeing this now. |
Description
A continuation of #3067 to split the iradon filter creation into a separate function and provide tests for the filters.
Checklist
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]
./doc/examples(new features only)[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: ]
For reviewers
(Don't remove the checklist below.)
later.
__init__.py.doc/release/release_dev.rst.