Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Aug 1, 2023

So it turns out the raw.decimate was unintentionally added by @alexrockhill in #10945 when refactoring TimeMixin. When I went to test it, it didn't work -- it relied on presence of a ._raw_times attribute which doesn't even exist for Raw instances. Same thing with SourceEstimate classes, which also errantly got .decimate methods -- they didn't work and I don't think were intended to be added. Since they never worked for these other classes, we can remove them without a deprecation cycle.

A future option would be to add proper support for them -- I actually did recently try to use raw.decimate because I had suitably filtered my data but ended up needing to use the much slower .resample instead! -- but it's not trivial. We jump through some substantial hoops in raw.resample to deal with concatenated instances, so not worth it for now.

Thus in this PR I:

  1. Put TimeMixin back to what it was, where the only public method was time_as_index
  2. Added all the stuff from [MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945 in a new class ExtendedTimeMixin(TimeMixin):
  3. Used the ExtendedTimeMixin as the mixin class for Epochs, Evoked, EpochsTFR, EvokedTFR, EDIT: and DipoleFixed. This means .decimate will only exist for those, which I think was the intended effect.
  4. Fixed a bug where .first was errantly being set on Epochs (this is an Evoked-only attribute)
  5. Properly tested epochs and evoked decim
  6. Properly tested AverageTFR decim (which was broken on main)
  7. Properly tested *TFR.shift_time which was available but untested on main

Closes #11797

@larsoner larsoner added this to the 1.5 milestone Aug 1, 2023
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @larsoner 🙏

@hoechenberger hoechenberger enabled auto-merge (squash) August 2, 2023 09:58
@hoechenberger hoechenberger merged commit a6c07cb into mne-tools:main Aug 2, 2023
@larsoner larsoner deleted the decim branch August 2, 2023 13:14
@alexrockhill
Copy link
Contributor

Thanks @larsoner and my bad for not thinking to check that TimeMixin was used in Raw. Thanks for fixing the bugs.

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
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.

Consider unifying raw.resample() and raw.decimate()

4 participants