Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Apr 2, 2025

I was annoyed at the prominent edge ringing when browsing raw data, see rightmost edge of this plot:

Screenshot From 2025-04-02 13-35-42

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 default padtype="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:

image

So this PR will need/has following things:

  • Clarifies differences between 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) unused thread= arg from _process_data
  • Removes a +1 that was changed to a +2 in an indexing calculation from ENH: Add time_format to Raw.plot() #9419.
  • I need to fully understand why this ☝️ was being done... I think it's probably not the right fix. Instead, somewhere the duration plotted should probably use a np.ceil instead of np.round on the number of samples plotted to ensure that the chosen duration is actually shown. That way when you have start, stop bounds you always get stop - start number of samples, not a kind of weird stop - start + 2, which is the behavior in main. (I think it was still being displayed correctly, but the code for it is unnecessarily complicated).
  • I need to update 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.
  • Revert larsoner branch uses in CIs here

I 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

)
else:
x_filtered = _overlap_add_filter(x_copy, h, n_fft, phase=phase)[0]
assert_allclose(x_filtered, x_expected, atol=1e-13)
Copy link
Member Author

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)

@larsoner
Copy link
Member Author

larsoner commented Apr 3, 2025

@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 larsoner/mne-qt-browser uses here

@larsoner larsoner added this to the 1.10 milestone Apr 8, 2025
@larsoner larsoner marked this pull request as ready for review April 8, 2025 19:33
@drammock
Copy link
Member

drammock commented Apr 8, 2025

This is a scheduled Ubuntu 20.04 brownout. Ubuntu 20.04 LTS runner will be removed on 2025-04-15. For more details, see actions/runner-images#11101

@larsoner
Copy link
Member Author

larsoner commented Apr 8, 2025

I can open a quick separate PR for that

@larsoner larsoner enabled auto-merge (squash) April 8, 2025 23:01
@larsoner larsoner merged commit 68f666e into mne-tools:main Apr 9, 2025
39 of 40 checks passed
@larsoner larsoner deleted the ringing branch April 9, 2025 15:51
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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants