Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EpochsTFRArray default drop log initialisation #13028

Merged
merged 6 commits into from
Dec 21, 2024

Conversation

tsbinns
Copy link
Contributor

@tsbinns tsbinns commented Dec 14, 2024

Reference issue (if any)

Fixes #13027

What does this implement/fix?

Expands the logic for EpochsTFRArray default drop_log initialisation so __getitem__() works with default params.

mne/time_frequency/tests/test_tfr.py Outdated Show resolved Hide resolved
"drop_log",
tuple(
() if k in self.selection else ("IGNORED",)
for k in range(max(len(self.events), max(self.selection) + 1))
Copy link
Member

Choose a reason for hiding this comment

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

scratching my head about this line. When would we not want just range(len(self.events))?

Copy link
Member

Choose a reason for hiding this comment

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

We always have len(self.events) == len(epochs). But let's say you originally had 100 events and just selected the last 10 on way or another, that resulting Epochs object shouldhave for example

len(epochs) == 10
len(epochs.selection) == 10
len(epochs.events) == 10
list(epochs.selection) == list(range(90, 100))

and in general for anything not in epochs.selection you should be able to query that index in epochs.drop_log to see why it's not part of epochs. So epochs.drop_log[30] for example should exist and be non-empty meaning it was dropped for a reason, whereas epochs.drop_log[90:] should exist and all be empty (i.e., those are the epochs that have been kept).

Copy link
Member

Choose a reason for hiding this comment

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

right, I forgot about events getting truncated/sliced/subselected.

Am I right then that we don't need to care about the case of having 30 events, and selecting only the middle 10? IIUC, if that happened via normal means, the drop_log would be present and the code in this diff won't be reached. It's only for EpochsTFRArray (where we don't know the dropping history) where we have to spawn a "fake" drop log, and in that case we can't know if (say) the last 10 epochs were dropped, but we also don't need to know that for things to work correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we just need some drop log that satisfies the necessary expectations about length of the drop log, selection values, and number of events.

Copy link
Contributor Author

@tsbinns tsbinns Dec 17, 2024

Choose a reason for hiding this comment

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

So something like

tuple(() if k in self.selection else ("IGNORED",) for k in len(self.events))

to account for the fact that a non-default selection param could be passed?

Or just the super simple

tuple(() for k in self.events)

Copy link
Member

Choose a reason for hiding this comment

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

No the len(self.events) won't be enough, I think what you have here with the max(...) is more correct

Copy link
Member

Choose a reason for hiding this comment

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

... or you have to make sure self.selection = np.arange(len(self.events))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. Yeah at the moment whatever selection can be passed so self.selection = np.arange(len(self.events)) can't be assumed.

@tsbinns
Copy link
Contributor Author

tsbinns commented Dec 19, 2024

Have now switched from as_array to as_tfr_array in the test, and have included a changelog entry.

@larsoner larsoner merged commit d814954 into mne-tools:main Dec 21, 2024
24 of 26 checks passed
@larsoner
Copy link
Member

Thanks @tsbinns !

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.

Selecting epochs from EpochsTFRArray objects raises obscure error due to incomplete drop_log initialisation
3 participants