ENH: Implement mode parameter for spline filtering. #8537
Conversation
jaimefrio
changed the title
| @@ -62,12 +63,24 @@ def spline_filter1d(input, order=3, axis=-1, output=numpy.float64): | |||
| output : ndarray or dtype, optional | |||
| The array in which to place the output, or the dtype of the returned | |||
| array. Default is `numpy.float64`. | |||
| %(mode)s | |||
There was a problem hiding this comment.
The actual text is in ni_docstrings.py, and this gets magically filled in by the @_ni_docstrings.docfiller decorator.
|
I wonder if it wouldn't help to change some of the functions to accept keywords? |
|
Sorry for taking so long, but I struggled a bit with getting the test in |
|
@charris Could you please take another look? |
|
This looks great. Not going to dive into the details, since it would take way too long to catch up with @jaimefrio and @charris here. Would be useful to have an entry in the release notes, seems an important enough improvement. @jaimefrio could you add a few words at https://github.com/scipy/scipy/wiki/Release-note-entries-for-SciPy-1.2.0 ? |
|
I'll take another look. IIRC, the code looked OK, my comments were directed at the docstrings. OK, added another docstring comment :) It is a surprisingly long time ago that I looked at this. @jaimefrio IIRC, there are some good tests for this, don't recall if they have been merged. I'm trying to avoid the work of digging through the files to check this in detail, in particular, I'd need to track down the source of the asymmetry between the causal and anti-causal initialization, for which I can think of several good, implementation dependent, reasons. |
|
@jaimefrio @charris okay to resolve the merge conflict and merge this before the 1.2.x split? seems good to go. I can address the last doc comment if needed |
|
I'm traveling this week and the next, and I won't get a chance to look into
this until I get back home, but feel free to take over this and put it in!
…On Sun, Oct 28, 2018 at 11:18 PM Ralf Gommers ***@***.***> wrote:
@jaimefrio <https://github.com/jaimefrio> @charris
<https://github.com/charris> okay to resolve the merge conflict and merge
this before the 1.2.x split? seems good to go. I can address the last doc
comment if needed
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8537 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADMGdqTXfFUnZIvFFbEhtOSDZyrZv_Z_ks5upp20gaJpZM4Sjh2h>
.
--
(\__/)
( O.o)
( > <) Este es Conejo. Copia a Conejo en tu firma y ayúdale en sus planes
de dominación mundial.
|
|
Hmm, for some reason I thought this had already gone in. I'll take another look, but I think it looked good to me before. If the tests are decent we should be able to rely on them. |
rgommers
force-pushed
the
spline_modes
branch
from
38772c8
to
1744e3a
Compare
- Some comment editing. - Have a first loop over the coefficients applying the filter gain, rather than doing it during the anticausal filtering, as it is better behaved numerically. - Rearranged arguments of _nd_image.spline_filter1d to have the new `mode` arg at the end.
rgommers
force-pushed
the
spline_modes
branch
from
1744e3a
to
5b70b91
Compare
|
Test coverage for changed functions looks good, and I pushed yet another rebase (all green except for Appveyor lag; previous run with same content was all green). Let's get this in. @charris if you want to give it another sanity check please do! Thanks @jaimefrio and @charris! |
This moves us forward along the lines discussed in #8465. None of the interpolation functions can yet use an image filtered with any mode other than
'mirror', but I think it's better to get this in first. If the interpolation functions have not been changed by the next release, we may want to change the docstring to not mislead users.This PR implements proper spline coefficient calculations for the
'mirror','wrap'and'reflect'modes, based on information obtained from Dr. Philippe Thévenaz. For'nearest'and'constant'methods this PR silently switches them to'mirror': the current implementation of the interpolation functions does a sufficiently good job for those methods, and they have some specific issues, so they didn't seem worth the trouble right now.