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

[MAINT] Add logic for handling AverageTFRArray in old MNE versions #185

Merged
merged 4 commits into from
May 24, 2024

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented May 23, 2024

Problem

Since MNE-Python v1.7 the API of time_frequency.AverageTFR has been changed. The old API from <= v1.6 is now available in the new class time_frequency.AverageTFRArray.

The example cwt_sensor_connectivity.py uses AverageTFR with the old API, causing documentation building to fail when using MNE-Python >= v1.7.

Solution

One possibility is to check whether the new class AverageTFRArray is available and import this (for >= v1.7), otherwise use the old AverageTFR (for v <= 1.6):

import mne
from mne import io
from mne_connectivity import spectral_connectivity_epochs, seed_target_indices
from mne.datasets import sample
# XXX: remove logic once support for mne<1.7 is dropped
try:
from mne.time_frequency import AverageTFRArray as AverageTFR
except ImportError:
from mne.time_frequency import AverageTFR

Also necessary to explicitly set the nave parameter, as the new AverageTFRArray does not support this as a positional argument:

tfr = AverageTFR(epochs.info, con.get_data(), times, freqs, nave=len(epochs))

Very open to other ideas for implementing such a logic. Cheers!

@tsbinns
Copy link
Collaborator Author

tsbinns commented May 23, 2024

Again some strange MacOS-specific runtime errors for code not touched by this PR like in #184.

Comment on lines 24 to 28
# XXX: remove logic once support for mne<1.7 is dropped
try:
from mne.time_frequency import AverageTFRArray as AverageTFR
except ImportError:
from mne.time_frequency import AverageTFR
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I'm not crazy about including logic like this in a public-facing example/tutorial. Is it enough to code it whatever way works for the CI docs build, and include a comment to the effect of "if you're on some other version of MNE, use the other class"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'm happy to take this approach as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you feel about this?

# Note that users of mne < 1.7 should use the `AverageTFR` class
tfr = AverageTFRArray(epochs.info, con.get_data(), times, freqs, nave=len(epochs))

@tsbinns
Copy link
Collaborator Author

tsbinns commented May 24, 2024

Unrelated spelling mistake just now being caught with updated codespell version addressed in #186.

@larsoner larsoner enabled auto-merge (squash) May 24, 2024 15:54
@larsoner
Copy link
Member

Looks like comments have been address so I'll mark for merge-when-green, thanks @tsbinns !

@larsoner larsoner merged commit 0b8d2af into mne-tools:main May 24, 2024
10 checks passed
@tsbinns tsbinns deleted the fix_tfr_object branch August 2, 2024 12:03
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.

3 participants