[MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945

alexrockhill · 2022-07-21T18:35:13Z

Fix leftovers from #10940.

So I thought, might as well loop in Evoked with this to be even more DRY but in doing so it turns out that they actually decimate in different ways. It comes down to the order of the following lines:

Evoked method:

self._decim *= decim
start_idx = int(round(-self._raw_times[0] * (self.info['sfreq'] *
                                                     self._decim)))

Epochs method:

start_idx = int(round(-self._raw_times[0] * (self.info['sfreq'] *
                                                     self._decim)))
self._decim *= decim

What this leads to is:

Original times:

array([-1. , -0.9, -0.8, -0.7, -0.6, -0.5, -0.4, -0.3, -0.2, -0.1,  0. ,
        0.1,  0.2,  0.3,  0.4,  0.5,  0.6,  0.7,  0.8,  0.9])

Evoked method:

array([-1. , -0.4,  0.2,  0.8])

Epochs method:

array([-0.6,  0.0,  0.6])

cc @agramfort @drammock @larsoner

Seems fairly consequential...

alexrockhill · 2022-07-21T18:54:22Z

Also there's a bit of an issue that the mixins docstring can only say one return type when self is returned which leads to issues like here https://mne.tools/stable/generated/mne.Evoked.html#mne.Evoked.shift_time where evoked says it returns an Epochs object.

alexrockhill · 2022-07-21T19:50:59Z

After a bit of digging, it seems like it's because the Epochs must have 0 because that's the sample that they are defined based on, which seems reasonable, it just would make sense that the Evoked would do the same.

alexrockhill · 2022-07-21T19:58:39Z

Ok this should fix all the things I found that were inconsistent.

alexrockhill · 2022-07-21T23:11:00Z

That looks pretty sensible to me for the multiple return type https://output.circle-artifacts.com/output/job/76871b03-098a-4353-9ac0-402f230ccd3b/artifacts/0/dev/generated/mne.Evoked.html?highlight=evoked#mne.Evoked.decimate, definitely an improvement over main.

alexrockhill · 2022-07-21T23:58:37Z

I realized I wasn't helping too much by putting another mixin that was redundant with TimeMixin so I just refactored everything within TimeMixin which really trimmed down a lot of redundant code. This got a bit more than I thought but I think this is a really nice refactoring.

alexrockhill · 2022-07-22T05:32:44Z

Ah shoot, times just has to be set with the _set_times setter one more place and the text of a warning changed, I'll fix it tomorrow morning.

drammock · 2022-07-22T13:40:17Z

That looks pretty sensible to me for the multiple return type https://output.circle-artifacts.com/output/job/76871b03-098a-4353-9ac0-402f230ccd3b/artifacts/0/dev/generated/mne.Evoked.html?highlight=evoked#mne.Evoked.decimate, definitely an improvement over main.

I'm not sure about the glossary entry for "time-series object". Try git grep "inst :" and you'll see lots of examples of how we handle this situation elsewhere. We're not perfectly consistent in how we do it, but let's not make that worse by adding a new variant.

drammock · 2022-07-22T13:41:13Z

cross-ref to #10947. Can you touch those two examples/tutorials in this PR, so they will render here and we can be sure they work again?

drammock

this looks like a nice refactoring. LMK when it's ready for another look.

doc/changes/latest.inc

drammock · 2022-07-22T13:49:27Z

mne/tests/test_evoked.py

@@ -81,7 +81,7 @@ def test_decim():
    info = create_info(n_channels, sfreq, 'eeg')
    with info._unlock():
        info['lowpass'] = sfreq_new / float(decim)
-    evoked = EvokedArray(data, info, tmin=-1)
+    evoked = EvokedArray(data, info)


why change this? I get that alignment will change now, so the exact times will change on the assert lines below, but an EvokedArray with negative tmin should still work.

It doesn't work with the immediately following tests with a simpler [::decim] because that doesn't account for the API change

I don't follow. The API change is that decim now shifts which samples it keeps in order to ensure that it keeps the sample at t=0. That shouldn't mean that there are no negative times.

If you have negative times then checking that the decimation worked properly is super complicated and done in the tests for epochs so better not to do that here.

ah, OK, I see now. The lines 88 and 89 were hidden from me in the GitHub PR review UI. If those lines are the issue, then it ought to be possible to revert the change to this line, and edit the starting sample (e.g., the 1 in 1::decim) in lines 88-89 right?

it is that simple. lines 84-90:

evoked = EvokedArray(data, info, tmin=-1) # back the way it started evoked_dec = evoked.copy().decimate(decim) evoked_dec_2 = evoked.copy().decimate(decim, offset=1) evoked_dec_3 = evoked.decimate(dec_1).decimate(dec_2) assert_array_equal(evoked_dec.data, data[:, 4::decim]) # just change the assert_array_equal(evoked_dec_2.data, data[:, 5::decim]) # starting samp assert_array_equal(evoked_dec.data, evoked_dec_3.data)

also, please stop marking conversations as "resolved" just because you disagree. Sometimes it's obvious (like "please make this an f-string") and it's fine, but in more complex cases, whether something is "resolved" is generally best determined by the person who asked the question.

-1 from me, the numbers 4 and 5 are picked out of nowhere, it's properly tested here:

raw, events, picks = _get_data() events = events[events[:, 2] == 1][:2] raw.load_data().pick_channels([raw.ch_names[pick] for pick in picks[::30]]) raw.info.normalize_proj() del picks sfreq_new = raw.info['sfreq'] / decim with raw.info._unlock(): raw.info['lowpass'] = sfreq_new / 12. # suppress aliasing warnings pytest.raises(ValueError, epochs.decimate, -1) pytest.raises(ValueError, epochs.decimate, 2, offset=-1) pytest.raises(ValueError, epochs.decimate, 2, offset=2) for this_offset in range(decim): epochs = Epochs(raw, events, event_id, tmin=-this_offset / raw.info['sfreq'], tmax=tmax, baseline=None) idx_offsets = np.arange(decim) + this_offset for offset, idx_offset in zip(np.arange(decim), idx_offsets): expected_times = epochs.times[idx_offset::decim] expected_data = epochs.get_data()[:, :, idx_offset::decim] must_have = offset / float(epochs.info['sfreq']) assert (np.isclose(must_have, expected_times).any()) ep_decim = epochs.copy().decimate(decim, offset) assert (np.isclose(must_have, ep_decim.times).any()) assert_allclose(ep_decim.times, expected_times) assert_allclose(ep_decim.get_data(), expected_data) assert_equal(ep_decim.info['sfreq'], sfreq_new) # More complicated cases epochs = Epochs(raw, events, event_id, tmin, tmax) expected_data = epochs.get_data()[:, :, ::decim] expected_times = epochs.times[::decim] for preload in (True, False): # at init epochs = Epochs(raw, events, event_id, tmin, tmax, decim=decim, preload=preload) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) # split between init and afterward epochs = Epochs(raw, events, event_id, tmin, tmax, decim=dec_1, preload=preload).decimate(dec_2) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) epochs = Epochs(raw, events, event_id, tmin, tmax, decim=dec_2, preload=preload).decimate(dec_1) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) # split between init and afterward, with preload in between epochs = Epochs(raw, events, event_id, tmin, tmax, decim=dec_1, preload=preload) epochs.load_data() epochs = epochs.decimate(dec_2) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) epochs = Epochs(raw, events, event_id, tmin, tmax, decim=dec_2, preload=preload) epochs.load_data() epochs = epochs.decimate(dec_1) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) # decimate afterward epochs = Epochs(raw, events, event_id, tmin, tmax, preload=preload).decimate(decim) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) # decimate afterward, with preload in between epochs = Epochs(raw, events, event_id, tmin, tmax, preload=preload) epochs.load_data() epochs.decimate(decim) assert_allclose(epochs.get_data(), expected_data) assert_allclose(epochs.get_data(), expected_data) assert_equal(epochs.info['sfreq'], sfreq_new) assert_array_equal(epochs.times, expected_times) # test picks when getting data picks = [3, 4, 7] d1 = epochs.get_data(picks=picks) d2 = epochs.get_data()[:, picks] assert_array_equal(d1, d2)

I meant that I thought I had addressed it but you're free to unresolve, sorry if that offended you.

it's not plucked from nowhere. 4 == evoked.times.tolist().index(0) - decim and 5 is because the second one has offset=1

evoked = EvokedArray(data, info, tmin=-1) zero_idx = evoked.times.tolist().index(0) evoked_dec = evoked.copy().decimate(decim) evoked_dec_2 = evoked.copy().decimate(decim, offset=1) evoked_dec_3 = evoked.decimate(dec_1).decimate(dec_2) start_samp = zero_idx - decim assert_array_equal(evoked_dec.data, data[:, start_samp::decim]) # this has +1 because offset=1 when decimating ↓↓↓↓↓↓↓↓↓↓↓↓↓↓ assert_array_equal(evoked_dec_2.data, data[:, (start_samp + 1)::decim])

you can change n_times on line 75 to whatever you like and this will still pass

mne/time_frequency/tests/test_tfr.py

mne/utils/mixin.py

alexrockhill · 2022-07-22T14:15:27Z

That looks pretty sensible to me for the multiple return type https://output.circle-artifacts.com/output/job/76871b03-098a-4353-9ac0-402f230ccd3b/artifacts/0/dev/generated/mne.Evoked.html?highlight=evoked#mne.Evoked.decimate, definitely an improvement over main.

I'm not sure about the glossary entry for "time-series object". Try git grep "inst :" and you'll see lots of examples of how we handle this situation elsewhere. We're not perfectly consistent in how we do it, but let's not make that worse by adding a new variant.

I looked but I think this is a new case for mixins where you don't have multiple input types accepted (because it's a method) and ideally you would just put the correct output type as the return but there's no way to know the class from the method docstring so I think this is the most elegant solution rather than having a huge list or for me even more confusing evoked methods that say they return epochs.

alexrockhill · 2022-07-22T16:31:56Z

Ok, I touched the failing examples and everything is green, should be good to go @drammock!

Unless you don't like the glossary entry but I looked pretty hard to find out how to get the method from the first inherited parent class and I don't think you can in python3 and so this is the best thing I could come up with which I think is better than what's on main (evoked returns epochs). The other option is a long list of return types but I think that's misleading as well because it will only return one type; the object that's method it is. I do really wish there were a way to infer the class from the method but it just doesn't appear to be possible, and the docstring is made only once for the parent class and then copied to the inherited classes so that seems to be insurmountable.

alexrockhill · 2022-07-25T22:12:11Z

Ok to merge @drammock?

Thanks for writing the more comprehensive tests!

mne/utils/mixin.py

larsoner · 2022-07-26T17:04:10Z

doc/conf.py

@@ -237,6 +237,7 @@
    'Transform': 'mne.transforms.Transform',
    'Coregistration': 'mne.coreg.Coregistration',
    'Figure3D': 'mne.viz.Figure3D',
+    'MNE-object': ':term:`MNE time-series object`',


I'd rather not do this in this PR. In most places we just do instance of Raw, Epochs, or Evoked. It's not optimal and in some cases not even complete (e.g., can be AverageTFR, STC, etc. instead) but it's common and people are used to it. If we want to go this route it should be in a separate PR where we change a ton of these at once. But for now I'd just make this instance of Raw, Epochs, Evoked, AverageTFR, or SourceEstimate or so.

But even separate-PR aside, I'm not totally convinced we need to or want to create this new category just for the purposes of making our Returns section better. For example, to me the best solution would be to somehow magically infer the class name being documented but I don't think this is possible based on how autodoc works. To me the simplest solution is to do something like instance of original type or something more compact that has this same meaning to say "you get back the same instance mostly for chaining purposes".

We should discuss this separately in a new issue. @alexrockhill if you're motivated by this problem, could you try making a new issue with the points that people have made about this already, then we can continue the discussion there?

+1 for instance of original type, it is a tricky corner case for autodoc. Sounds good, I'll remove.

... but for now in this PR please just use a list and open an issue where we discuss separately what the correct verbiage/linking should be for this "original type" stuff would be

* upstream/main: FIX: ctf data can sometimes have no date time. (mne-tools#10957) replace check_version from _testing.py with the one in check.py (mne-tools#10958) ENH: mne-tools#10542 Adds __eq__() to DigMontage class and updates tests. (mne-tools#10942) Use HiDPI icons with PyQt5/PySide2 (mne-tools#10956)

larsoner · 2022-07-27T15:09:39Z

Thanks @alexrockhill !

alexrockhill changed the title ~~[MAINT, WIP] Ignore filter warning for decim for EpochsTFR~~ [MAINT, MRG] Ignore filter warning for decim for EpochsTFR Jul 21, 2022

alexrockhill changed the title ~~[MAINT, MRG] Ignore filter warning for decim for EpochsTFR~~ [MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR Jul 22, 2022

drammock reviewed Jul 22, 2022

View reviewed changes

alexrockhill mentioned this pull request Jul 22, 2022

[ENH] Add tutorial on time-frequency source estimation with STC viewer GUI #10920

Merged

4 tasks

alexrockhill added 11 commits July 25, 2022 10:24

wip

02145d8

epochs and evoked used different decim

b97ca27

temporary workaround

4c911e1

fix return type

80beeb1

fix aforementioned things

980b93b

fix ref

c837e89

wip

d0d159e

fix test, more refactoring

49829d8

fix flake

aa1bf70

fix last things, touch tutorials, should be fixed

a9e0f6c

dan addition

6ce73b3

alexrockhill force-pushed the fix branch from e257033 to 6ce73b3 Compare July 25, 2022 17:24

mscheltienne reviewed Jul 26, 2022

View reviewed changes

mne/utils/mixin.py Show resolved Hide resolved

larsoner requested changes Jul 26, 2022

View reviewed changes

larsoner mentioned this pull request Jul 26, 2022

Adding an argument include_tmax for .get_data() #10372

Open

larsoner added 2 commits July 27, 2022 10:06

DOC: Remove for now

39b464b

larsoner mentioned this pull request Jul 27, 2022

DOC: Document inst better #10965

Open

larsoner enabled auto-merge (squash) July 27, 2022 14:14

larsoner added this to the 1.1 milestone Jul 27, 2022

This was referenced Jul 27, 2022

BUG: Test failures on M1 with BLAS from conda-forge #10759

Closed

Releasing MNE-Python 1.1 #10709

Closed

larsoner approved these changes Jul 27, 2022

View reviewed changes

larsoner disabled auto-merge July 27, 2022 15:09

larsoner merged commit 23f37cd into mne-tools:main Jul 27, 2022

drammock mentioned this pull request Jul 28, 2022

build-docs-main failure #10947

Closed

larsoner mentioned this pull request Aug 1, 2023

BUG: Only add decimate to correct classes #11853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945

[MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945

alexrockhill commented Jul 21, 2022 •

edited

Loading

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 22, 2022 •

edited

Loading

drammock commented Jul 22, 2022

drammock commented Jul 22, 2022

drammock left a comment

drammock Jul 22, 2022

alexrockhill Jul 22, 2022

drammock Jul 22, 2022

alexrockhill Jul 22, 2022

drammock Jul 22, 2022

drammock Jul 22, 2022

drammock Jul 22, 2022

alexrockhill Jul 22, 2022

drammock Jul 22, 2022

drammock Jul 22, 2022 •

edited

Loading

alexrockhill commented Jul 22, 2022

alexrockhill commented Jul 22, 2022 •

edited

Loading

alexrockhill commented Jul 25, 2022 •

edited

Loading

larsoner Jul 26, 2022

alexrockhill Jul 26, 2022

larsoner Jul 26, 2022

larsoner commented Jul 27, 2022

[MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945

[MAINT, MRG] Refactor TimeMixin to include more time operations, ignore filter warning for decim for EpochsTFR #10945

Conversation

alexrockhill commented Jul 21, 2022 • edited Loading

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 21, 2022

alexrockhill commented Jul 22, 2022 • edited Loading

drammock commented Jul 22, 2022

drammock commented Jul 22, 2022

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drammock Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

alexrockhill commented Jul 22, 2022

alexrockhill commented Jul 22, 2022 • edited Loading

alexrockhill commented Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner commented Jul 27, 2022

alexrockhill commented Jul 21, 2022 •

edited

Loading

alexrockhill commented Jul 22, 2022 •

edited

Loading

drammock Jul 22, 2022 •

edited

Loading

alexrockhill commented Jul 22, 2022 •

edited

Loading

alexrockhill commented Jul 25, 2022 •

edited

Loading