Speed up evoked time plotting - #14249
Conversation
|
@wmvanvliet okay to merge or did you need to test / look further? |
* upstream/main: (22 commits) Avoid copying all epochs data in GetEpochsMixin._getitem (mne-tools#14262) Add Report.save(only_if_changed=True) (mne-tools#14261) Add jamica to related software [ci skip] (mne-tools#14260) Remove debugging cruft (mne-tools#14259) ENH: add Raw annotation span conversion (mne-tools#14240) Interactive dipole fitting: add STC mesh controls (mne-tools#14256) Document code principles in AGENTS.md (mne-tools#14239) MAINT: Update dependency specifiers (mne-tools#14257) [dependabot]: Bump the actions group with 2 updates (mne-tools#14258) ENH: Add JAMICA as an ICA method (mne-tools#14247) Reuse the MEF session across reads (mne-tools#14254) Read KIT data in cache-sized blocks (mne-tools#14255) Read EGI simple-binary event channels in blocks (mne-tools#14250) Decode Persyst and Nihon Kohden data in cache-sized blocks (mne-tools#14251) Normalize byte order before calibrating strided integer buffers (mne-tools#14252) Speed up EDF and BDF reading (mne-tools#14237) ENH: Add Forward-based projection reconstruction (mne-tools#14235) Read Artemis123, Curry, EEGLAB, Eximia, FIL and NSx in cache-sized blocks (mne-tools#14246) Remove rotating dipoles capability from interactive dipole fitting GUI (mne-tools#14243) Read BrainVision data in cache-sized blocks [ci skip] (mne-tools#14241) ...
|
Could you explain a bit some of the changes in this PR? It looks like MNE-Python need to work around blitting bugs in matplotlib of something. Why is the coding dealing with low-level image functions? |
|
Sure -- this PR was targeting blitting and speed, so I decided to tack on a few other loosely related fixes that could in principle be separated into separate PRs if needed. 1.
|
|
I appreciate the explanation, now I know why this is way more complicated then I thought it would be. |
|
Want me to split it up? Easy enough for me to do |
wmvanvliet
left a comment
There was a problem hiding this comment.
Just trying to follow all the logic. But it looks good, no need to split this up into smaller PRs I think.
| # Raw RGBA: the pixels are all that is wanted below, so encoding them as PNG | ||
| # only to decode them again is pure overhead. |
There was a problem hiding this comment.
Now that things have moved around, this comment seems out of place. At this particular point, the code is clear: the caller asked for image_format="rgba" so that is what we will use. I think this comment needs to be further down (see my other comment)
| pil_kwargs.update(optimize=True, compress_level=9) | ||
| output.seek(0) | ||
| orig = Image.open(output) | ||
| if mpl_format == "rgba": |
There was a problem hiding this comment.
Here is were we distinguish between whether the image was converted to svg/png or not and hence needs to be decoded. So this might be the place for a comment like "grab the pixels straight from the canvas"
|
Fixed, thanks for the review @wmvanvliet ! Marking for merge-when-green |
* upstream/main: (37 commits) Speed up evoked time plotting (mne-tools#14249) Persist the numba JIT cache, sysmon coverage, refleak freeze (mne-tools#14265) Fix interpolate_to spline target positions (mne-tools#14266) CI reduce package builds on pull requests (mne-tools#14264) Avoid copying all epochs data in GetEpochsMixin._getitem (mne-tools#14262) Add Report.save(only_if_changed=True) (mne-tools#14261) Add jamica to related software [ci skip] (mne-tools#14260) Remove debugging cruft (mne-tools#14259) ENH: add Raw annotation span conversion (mne-tools#14240) Interactive dipole fitting: add STC mesh controls (mne-tools#14256) Document code principles in AGENTS.md (mne-tools#14239) MAINT: Update dependency specifiers (mne-tools#14257) [dependabot]: Bump the actions group with 2 updates (mne-tools#14258) ENH: Add JAMICA as an ICA method (mne-tools#14247) Reuse the MEF session across reads (mne-tools#14254) Read KIT data in cache-sized blocks (mne-tools#14255) Read EGI simple-binary event channels in blocks (mne-tools#14250) Decode Persyst and Nihon Kohden data in cache-sized blocks (mne-tools#14251) Normalize byte order before calibrating strided integer buffers (mne-tools#14252) Speed up EDF and BDF reading (mne-tools#14237) ...
Closes #14242
@wmvanvliet I generalized our blit code. Looks like it's actually pretty clean, steps:
brainblit code to a more general version_BlitManagerevoked.plotandevoked.plot_topoMinor changes to evoked.plot and evoked.topo are that the time
axvlinewill stay on top now, but I think that's reasonable (and expected).(5) was a pain to discover -- turns out to be caused by the modification of the contours, which removes and re-adds the set to the axes, which invalidates the draw, causing a full redraw (with nothing left to draw, so you get a blank plot). The workaround I came up with is to add a dummy no-op
axclass pass to thecontourcall, and then pull the contours from it. I'm going to work on upstreaming a warning about this and a less-hacky way to get the contours... but it might be a bit to land etc. In the meantime I think the no-op class is workable, and has a big performance boost.(5) speeds up
report.add_evokedfrom about 8s to 5s by dropping the timepoint code from over 3s to under 1s.I then also sped up how images are embedded in reports to cut down another second off that 5s
add_evokeds.I used Claude Opus 5 to investigate (iterating / reviewing multiple versions).