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

[GSOC] Store multivariate connectivity filters #192

Closed
wants to merge 4 commits into from

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented May 28, 2024

Problem

One requirement of the decoding module we will introduce in the GSoC project is access to the multivariate filters, which are currently computed in the connectivity estimator class, but never stored. Discussed in more detail in #182.

I thought submitting this as a separate PR from the wider decoding module would help keep diff size small and make reviewing the changes easier.

Solution

Add a filters attribute to the multivariate connectivity estimator classes similar to the existing patterns attribute.

However, since this will only be required in the case of the connectivity estimator being used in the decoding module (not with spectral_connectivity_epochs/time() functions), also add a store_filters flag which is by default False.

coherency (MIC) and multivariate interaction measure (MIM): Ewald et al.
(2012). NeuroImage. DOI: 10.1016/j.neuroimage.2011.11.084
- Coherency/coherence, i.e. canonical coherency (CaCoh): Vidaurre et al.
(2019). NeuroImage. DOI: 10.1016/j.neuroimage.2019.116009
"""

When store_filters=True, the filters attribute will be set as a placeholder array where filters can be stored (again, similar to how patterns works), however when False, filters will remain as None to reduce memory usage.

csd = self.reshape_csd() / n_epochs
n_times = csd.shape[0]
times = np.arange(n_times)
freqs = np.arange(self.n_freqs)

It would be sufficient to pass store_filters to the _MultivariateCohEstBase estimator class and have the filters attribute there, however I have passed store_filters to the parent _EpochMeanMultivariateConEstBase class and have the filters attribute here. This keeps things in line with the patterns attribute which is also currently only used for daughters of the _MultivariateCohEstBase, but this may not be the case in the future if other filter-based non-coherency methods are implemented, so this approach is more flexible and forward-looking.

class _EpochMeanMultivariateConEstBase(_AbstractConEstBase):
"""Base class for mean epoch-wise multivar. con. estimation methods."""
n_steps = None
patterns = None
filters = None
con_scores_dtype = np.float64
def __init__(
self, n_signals, n_cons, n_freqs, n_times, n_jobs=1, store_filters=False
):
self.n_signals = n_signals
self.n_cons = n_cons
self.n_freqs = n_freqs
self.n_times = n_times
self.n_jobs = n_jobs
self.store_filters = store_filters

Other than that, it's a very simple case of just storing the filters when appropriate.
MIC

if self.store_filters:
self.filters[0, con_i, :n_seeds] = alpha_Ubar
self.filters[1, con_i, :n_targets] = beta_Ubar

CaCoh

if self.store_filters:
self.filters[0, con_i, :n_seeds] = alpha_Ubar
self.filters[1, con_i, :n_targets] = beta_Ubar

Since store_filters is by default False, no behaviour has been changed. Since this is also an internal feature there are no new unit tests to add at this stage (would all come when the decoding module is implemented proper).

Other points

When implementing this, I also realised it was also possible to refactor the reshape_results() method in the multivariate connectivity estimator classes which were separately implemented for the coherency and Granger methods but doing the same thing, so that has been cleaned up.

def reshape_results(self):
"""Remove time dimension from results, if necessary."""
if self.n_times == 0:
self.con_scores = self.con_scores[..., 0]
if self.patterns is not None:
self.patterns = self.patterns[..., 0]
if self.filters is not None:
self.filters = self.filters[..., 0]

@tsbinns
Copy link
Collaborator Author

tsbinns commented May 29, 2024

Even if there is currently no functional change, I guess it is worth holding off on merging this until v0.7 is released (#181)??

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Agreed let's cut 0.7 then merge

@tsbinns tsbinns closed this Jun 19, 2024
@tsbinns
Copy link
Collaborator Author

tsbinns commented Jun 19, 2024

Changes we added in #193

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.

2 participants