-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ENH: Use data-based padding instead of "odd" padding when filtering in raw.plot #13183
base: main
Are you sure you want to change the base?
Conversation
) | ||
else: | ||
x_filtered = _overlap_add_filter(x_copy, h, n_fft, phase=phase)[0] | ||
assert_allclose(x_filtered, x_expected, atol=1e-13) |
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.
This is all just unindenting (and maybe 1 or 2 trivial cleanups I think)
@drammock I think this one is ready for review. If the ideas look reasonable we can review and merge mne-tools/mne-qt-browser#320 first, I can cut a release there, and then revert the |
I was annoyed at the prominent edge ringing when browsing raw data, see rightmost edge of this plot:
I originally decided to combat this by using
padtype="constant"
, but that wasn't quite good enough. I then tried making it so that even for IIR filters we could use our defaultpadtype="reflect_limited"
, and this helped, but again not quite enough. It also showed that there were slight differences between our"reflect_limited"
and NumPy's"reflect"
, even when there were sufficient samples. Even"reflect"
didn't get rid of the ringing entirely, which makes sense for the MEG data I was looking at where very high-frequency, high-amplitude cHPI frequencies cause sharp edges for all of these chosen pad types.So the ideal/correct fix is really to load sufficient data before and after the viewed window, then filter, then crop back to what you want to display (and do stuff like remove DC from that, choose min/max, decimate, etc.). With that behavior we get something much nicer:
So this PR will need/has following things:
np.pad(..., padtype="reflect")
and_smart_pad(..., "reflect_limited")
via unit tests. There is really just 1 sample difference, and you could make arguments for either as being more correct. Ours works slightly better for existing tests so let's keep it as-is.Removes a (seemingly) unusedthread=
arg from_process_data
+1
that was changed to a+2
in an indexing calculation from ENH: Add time_format to Raw.plot() #9419.np.ceil
instead ofnp.round
on the number of samples plotted to ensure that the chosenduration
is actually shown. That way when you havestart, stop
bounds you always getstop - start
number of samples, not a kind of weirdstop - start + 2
, which is the behavior inmain
. (I think it was still being displayed correctly, but the code for it is unnecessarily complicated).mne-qt-browser
to allow_process_data
to accept the modified*args
and future proof it against stuff like this, and then cut a bugfix release.larsoner
branch uses in CIs hereI don't think there are too many (any?) tests to add here since it's really hard to quantify the edge ringing. So hopefully just looking