[GSOC] Add support for multiple components of multivariate connectivity #213
Conversation
mne_connectivity/base.py
Outdated
| if self.method in ["cacoh", "mic", "mim", "gc", "gc_tr"]: | ||
| # multivariate results cannot be returned in a dense form as a | ||
| # single set of results would correspond to multiple entries in | ||
| # the matrix, and there could also be cases where multiple | ||
| # results correspond to the same entries in the matrix. | ||
| if ( | ||
| isinstance(self.indices, tuple) | ||
| and not np.all(isinstance(self.indices[0], int)) | ||
| and not np.all(isinstance(self.indices[1], int)) | ||
| ): # i.e. check if multivariate results based on nested indices | ||
| # multivariate results cannot be returned in a dense form as a single | ||
| # set of results would correspond to multiple entries in the matrix, and | ||
| # there could also be cases where multiple results correspond to the | ||
| # same entries in the matrix. | ||
| raise ValueError( | ||
| "cannot return multivariate connectivity data in a dense form" | ||
| ) |
There was a problem hiding this comment.
When updating the container unit tests I realised using the indices format is more a consistent check for multivariate conn scores rather than the method name
| # set n_components if necessary / check it is valid for the rank | ||
| if self.n_components is None: | ||
| self.n_components = np.min(rank) | ||
| elif self.n_components > np.min(rank): | ||
| raise ValueError( | ||
| "`n_components` is greater than the minimum rank of the data" | ||
| ) | ||
|
|
||
| # set rank if necessary | ||
| if self._rank is None: | ||
| self._rank = rank | ||
| # check n_components is valid for the rank | ||
| self.n_components = _check_n_components_input(self.n_components, self._rank) |
There was a problem hiding this comment.
Little refactoring change since this check is required also in spec_conn_epochs and spec_conn_time now
| @@ -480,113 +480,102 @@ def test_spectral_connectivity(method, mode): | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("method", ["cacoh", "mic", "mim", _gc]) | |||
| def test_spectral_connectivity_epochs_multivariate(method): | |||
| @pytest.mark.parametrize("n_components", [1, 2, 3, None]) | |||
| def test_spectral_connectivity_epochs_multivariate(method, n_components): | |||
There was a problem hiding this comment.
In addition to adding checks for multiple comps in this spec_conn_epochs unit test, I went ahead with some other improvements to clean up the code and make it generally more extensive.
Also, the simulated data being used here was a bit limiting to what we could test with multiple comps, so I switched from the old data simulation to the new make_signals_in_freq_bands() func, so ticks off some maintenance stuff too.
| bad_numpy_input = np.zeros((3, 3, 4, 5)) | ||
| bad_numpy_input = np.zeros((3, 3, 4, 5, 6)) | ||
| bad_indices = ([1, 0], [2]) | ||
|
|
||
| if conn_cls.is_epoched: | ||
| bad_numpy_input = np.zeros((3, 3, 3, 4, 5)) | ||
| bad_numpy_input = np.zeros((3, 3, 3, 4, 5, 6)) |
There was a problem hiding this comment.
For container tests, new comps dim increases the number of possible dimensions
|
Multiple comps now also supported for |
Co-authored-by: Eric Larson <[email protected]>
|
Sounds good, let me know when it's ready for another look! |
|
@larsoner given what we discussed on Weds about the plotting functionality not actually being impacted by these changes, I think everything we sought to address in this PR is here. So pending a review she is hopefully good to go! (I added comments throughout to code with short descriptions of the changes to try and make things less obtuse) |
There was a problem hiding this comment.
Just minor stuff from me, @wmvanvliet did you want to look or happy if I merge once my last comments are addressed?
Co-authored-by: Eric Larson <[email protected]>
…ectivity into decoding_ncomps
|
Looked it over and I am happy with this. Go ahead and merge! |
|
Thanks @tsbinns ! |
General idea described in #182 & #183.
So far support is implemented for multiple components of CaCoh and MIC in the decomposition class.
WIP for the
spectral_connectivity_...functions.