ENH: Use data-based padding instead of "odd" padding when filtering in raw.plot#13183
Merged
larsoner merged 23 commits intomne-tools:mainfrom Apr 9, 2025
Merged
ENH: Use data-based padding instead of "odd" padding when filtering in raw.plot#13183larsoner merged 23 commits intomne-tools:mainfrom
larsoner merged 23 commits intomne-tools:mainfrom
Conversation
larsoner
commented
Apr 3, 2025
| ) | ||
| else: | ||
| x_filtered = _overlap_add_filter(x_copy, h, n_fft, phase=phase)[0] | ||
| assert_allclose(x_filtered, x_expected, atol=1e-13) |
Member
Author
There was a problem hiding this comment.
This is all just unindenting (and maybe 1 or 2 trivial cleanups I think)
Member
Author
|
@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 |
Member
|
Member
Author
|
I can open a quick separate PR for that |
drammock
approved these changes
Apr 8, 2025
larsoner
added a commit
to SYXiao2002/mne-python
that referenced
this pull request
Apr 18, 2025
* upstream/main: (40 commits) fix typo (missing space) that messed up rst rendering (mne-tools#13217) MAINT: Restore VTK dev (mne-tools#13214) [pre-commit.ci] pre-commit autoupdate (mne-tools#13212) BUG: Fix bug with example (mne-tools#13210) MAINT: Fix pip-pre with PyVista (mne-tools#13207) Move FCBG to former partners (mne-tools#13205) ENH: Update related software list (mne-tools#13202) fix sfreq estimation for snirf files (mne-tools#13184) ENH: Use data-based padding instead of "odd" padding when filtering in raw.plot (mne-tools#13183) FIX: Bumps (mne-tools#13198) DOC: fix typo in examples/io/read_impedances.py (mne-tools#13197) [pre-commit.ci] pre-commit autoupdate (mne-tools#13173) FIX make_watershed_bem to handle missing talairach_with_skull.lta courtesy Freesurfer 8 (mne-tools#13172) ENH: Add upsampling for MEG helmet surface (mne-tools#13179) MAINT: Update code credit (mne-tools#13180) BUG: Fix bug with least-squares sphere fit (mne-tools#13178) fix EDF export (mne-tools#13174) fix typo (mne-tools#13171) [pre-commit.ci] pre-commit autoupdate (mne-tools#13164) Fix dev installation guide (mne-tools#13163) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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+1that was changed to a+2in an indexing calculation from ENH: Add time_format to Raw.plot() #9419.np.ceilinstead ofnp.roundon the number of samples plotted to ensure that the chosendurationis actually shown. That way when you havestart, stopbounds you always getstop - startnumber 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-browserto allow_process_datato accept the modified*argsand future proof it against stuff like this, and then cut a bugfix release.larsonerbranch 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