mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
68 stars 34 forks source link

[ENH] Add connectivity simulation function #173

Closed tsbinns closed 6 months ago

tsbinns commented 7 months ago

Summary

Adds a new public function make_signals_in_freq_bands() inside mne_connectivity/datasets/frequency.py for a basic simulation of connectivity, following @adam2392's suggestion in #163.


API/Documentation

The function has parameters for what I feel are the most important simulation options.

I added a new "Dataset functions" section to the API page since I wasn't sure how well it fit under the existing headers and link to the function here. Very open to change this if you have better suggestions.


Unit tests

I added a new set of tests in mne_connectivity/tests/test_datasets.py.

test_make_signals_error_catch() checks that some obvious errors are caught. If you think there are some important checks I've missed please let me know.

test_make_signals_in_freq_bands checks that the simulations are actually working properly. I focused on Coh, ImCoh, and DPLI with: 1) no time delay in interaction between seeds and targets; 2) a positive time delay (i.e. seeds drive communication with targets); and 3) a negative time delay (i.e. targets drive communication with seeds). From this we expect:

I test these with a higher and lower SNR, different number of seeds and targets, and the different CSD computation modes. I realise it's not an exhaustive check, but I think the tests nicely capture what we expect to see from undirected (Coh, ImCoh) and directed (DPLI) connectivity methods, and also methods that do not capture zero time-lag interactions (ImCoh).


Use elsewhere in the package

This simulation function is similar (but not identical) to those used for many of the unit tests in spectral.py. Out of interest I wanted to see if this function could be used to replace these simulations. In a number of cases it works, but in others it would require redefining what is considered acceptable connectivity values for particular method/CSD mode combinations.

This could be done, but it would require a considerable amount of effort tweaking these thresholds which I can't really invest the time for at the moment.

How would people feel about just having this new function which can be used going forward for e.g. the CaCoh examples in #163 without changing the existing unit test stimulations?

larsoner commented 7 months ago

This could be done, but it would require a considerable amount of effort tweaking these thresholds which I can't really invest the time for at the moment

Can you just add a # TODO: Use <whatever> for this after tweaking tolerances so we don't forget that this is a possibility? Seems okay to leave it to future work

adam2392 commented 7 months ago

How would people feel about just having this new function which can be used going forward for e.g. the CaCoh examples in #163 without changing the existing unit test stimulations?

Makes sense to me!

tsbinns commented 7 months ago

@larsoner @adam2392 Thanks both for the feedback!

I replaced the simulations where no values were being checked (e.g. for error catching parameters, saving & loading, etc...), and marked 5 tests where the simulations could be replaced with make_signals_in_freq_bands following some tweaks to the acceptable values.

tsbinns commented 7 months ago

Looks like some of the thresholds were just on the boundary and updating the way in which the random numbers were generated caused these to fail, so tweaked the thresholds slightly.

adam2392 commented 7 months ago

I'll let @larsoner and @drammock take a quick look.

Perhaps @ruuskas is also interested given he found a bug with the previous implementation? Hopefully this will simplify life for generating simulated data with expected correlations in the frequency domain

adam2392 commented 6 months ago

Perhaps something to help move this forward is refactoring #163 to use this function? (I.e. merge changes might work?...)

I would keep a local copy in case git messes up.

Then we can see the simulation function work as intended for the new PR?

If that's a bit hairy, I'm okay merging for now. Wdyt @larsoner

tsbinns commented 6 months ago

@adam2392 Sure, I'm open to this. Will be away at a conference rest of this week, but can start on Monday.

adam2392 commented 6 months ago

No rush! Enjoy!

tsbinns commented 6 months ago

@adam2392 I've refactored #163 to use the new simulation function in the CaCoh and coherency method comparison examples where I was previously using a function within each example file. Everything still works, no problems switching to the dedicated function.

adam2392 commented 6 months ago

Ah very cool! I think this LGTM as a start to simplifying data generation in our connectivity analyses.