-
Notifications
You must be signed in to change notification settings - Fork 57
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] Change to Linear for upsampling #820
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #820 +/- ##
=======================================
Coverage 28.12% 28.12%
=======================================
Files 69 69
Lines 10079 10079
Branches 1302 1302
=======================================
Hits 2835 2835
Misses 7146 7146
Partials 98 98 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Taylor Salo <[email protected]>
Why do you use Lanczos windowed sinc for downsampling and upsampling to >90% of the original voxel size. Would it make more sense to just use linear no matter what? |
Lanczos Sinc is very accurate if certain conditions are met. As long as there aren't sharp cutoffs between high and low intensity and the image resolution in the output isn't higher than the input data it is a great default (and is the default in fmriprep). A added the 90% as some wiggle room for weird voxel sizes. Linear intepolation ends up smoothing the data, which is not something we want to do unintentionally. I think it makes sense for upsampling, since it won't introduce the ringing effects like a Sinc interpolator will. I didn't think BSpline would add ringing (particularly the relatively simple BSpline[3] we were using), but in some cases it looks like can. Although, this may only happen if there is significant upsampling. I'm not sure what the native resolution was of the data that showed the ringing artifacts. |
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.
LGTM
Addresses #819