ENH: Add multiple label support to source_band_induced_power, source_induced_power

[nordme]

Fixes #11985.

What does this implement/fix?

source_induced_power and source_band_induced_power currently only allow one label to be passed for power and plv calculation. It should be able to do this over multiple labels.

[larsoner]

Great! Next (before adding anything else) I would add a test that return_plv actually makes the pow, itc tuple get returned. Don't actually need to check the values but you could for example make sure that their data shapes are the same

[nordme]

Let's check in about how we want to handle output for single labels and any other main feedback points about restricting K and then averaging power over labels. After we've got that shored up, then I will need to do work on the tests.

[nordme]

The most recent commit proposes performing kernel restriction to labels within the _prepare_source_params function, but if we do that, then we need to handle the _compute_source_psd_epochs function as well. So, at this point, let's touch base on a conceptual level about if we want to move multi-label restriction into _source_induced_power so that we avoid changes to psd functions or if we want to look at multi-label functionality for the psd epochs function too.

If we want to continue on with _prepare_source_params then I need to finish the alterations to that function and clean it up (e.g. excessive comments), but I don't want to move ahead until we've weighed the overall strategy.

[larsoner]

if we do that, then we need to handle the _compute_source_psd_epochs function as well.

The current public API of https://mne.tools/stable/generated/mne.minimum_norm.compute_source_psd_epochs.html says it only supports a single label. So the easy way out is to still only allow that behavior in that function, but add in a code comment somewhere # TODO: Now that multiple labels are supported in _prepare_source_params, it should be easy to add multi-label support here and someone else can implement it later if they want. Then your job now is just to add this comment and catch in compute_source_psd_epochs (e.g., with _validate_type(label, (None, Label), label)) that only a single label is passed. Then the simple/equivalent-to-main code path of _prepare_source_params will be triggered so the output and behavior of compute_source_psd_epochs remains unchanged.

... or if you want to go into bonus territory, yes you make compute_source_psd_epochs also support multiple labels, with the same tutorial and testing updates to make sure things are still working there. Depends on how strong your motivation (and how open your time) is to work on this :)

[nordme]

Thanks, @larsoner! About single label behavior: do you think the _source_induced_power should continue to output power as (n_vertices, n_freqs, n_samples) if there's only a single label so that old scripts using this function are not broken? It would be kinda weird, but it would not break people's existing pipelines?

[larsoner]

do you think the _source_induced_power should continue to output power as (n_vertices, n_freqs, n_samples) if there's only a single label so that old scripts using this function are not broken? It would be kinda weird, but it would not break people's existing pipelines?

Yes exactly. We keep backward compat that way. And to me it's okay (even though it's a bit awkward) that label=label will give (n_vertices, n_freqs, n_samples) whereas label=[label] will give (1, n_freqs, n_samples). To me it's two different modes of operation, the first giving all vertices within a label (single-label input) and the second giving label-meaned/aggregated data (list of labels).

[nordme]

@larsoner Looks like pre-commit hooks fixed a whitespace issue that's now causing CircleCi to fail (unregistered user issue). Can I fix this or does it need to be you?

[larsoner]

You can fix it and push a commit

[nordme]

@larsoner There's nothing to fix in terms of the code, so am I pushing an empty commit to get CircleCI to restart?

[larsoner]

Yes you can but these style CIs are red, too

https://github.com/mne-tools/mne-python/actions/runs/6550989358/job/17791250636?pr=12026
https://results.pre-commit.ci/run/github/1301584/1697565252.Q-17qBRiRZa0i7bLzB1HGA

[nordme]

@larsoner I'm puzzled about the 4 failed CircleCi tests. It looks to me like they're failing on code that a) I didn't touch, b) doesn't call code that I touched, and c) passes pytest on my machine locally. Do you have thoughts on what's happening?

[drammock]

• pip pre errors are related to numpy API changes it looks like
• ubuntu minimal error is in test_tfr_multi_label which looks related?: `E FileNotFoundError: fname does not exist: "/home/runner/work/mne-python/mne-python/MEG/sample/sample_audvis_trunc_raw.fif"

[nordme]

@drammock Thanks, Dan. About the multi-label one, my raw instantiation call is the same as the one above it in test_tfr_with_inverse_operator(method). Do I need the decorator @testing.requires_testing_data above my multi-label test function to make it work? Is that an issue that would normally pass pytest on my machine but break CircleCI checks?

[drammock]

@drammock Thanks, Dan. About the multi-label one, my call is the same as the one above it in test_tfr_with_inverse_operator(method). Do I need the decorator @testing.requires_testing_data above my multi-label test function to make it work? Is that an issue that would normally pass pytest on my machine but break CircleCI checks?

yes indeed. Look at the step in which the failure occurs: https://github.com/mne-tools/mne-python/actions/runs/6551626097/job/17793283214?pr=12026

it's title is "Run tests with no testing data". adding the decorator would cause the test to get skipped on that CI job.

[larsoner]

One major suggestion for refactoring/restructuring. If it's not clear let me know and I'm happy to chat about it, or if you want I can push a commit to try it

[nordme]

One major suggestion for refactoring/restructuring. If it's not clear let me know and I'm happy to chat about it, or if you want I can push a commit to try it

@larsoner Sounds good, exploiting the power of dicts more here is a good idea. I'll work on it and push, probably tomorrow!

[nordme]

@drammock @larsoner I think this PR is pretty much ready, but do either of you have last comments?

[larsoner]

Just some tiny stuff and one remaining choice, then I think we can merge!

[nordme]

@larsoner Looks like nibabel errors are causing the CircleCi failures?

[larsoner]

Pushed a tiny commit to make CIs happy (failures are unrelated but might as well get them fixed now!) and marking for merge-when-green, thanks @nordme !