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

[BUG] Fix memory error spectral_connectivity_time #175

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Mar 6, 2024

Fix for #174.

In short, the array for storing spatial patterns of connectivity used by the MIC method was unnecessarily being created for all methods. This could lead to memory errors when bivariate connectivity methods were being called on large numbers of connections.

This fix makes it so that the patterns array will only be instantiated in methods that require it, otherwise will be set to None. This does not cause any change in behaviour, since for methods where patterns are not used, None was already being stored in the patterns attribute of the returned connectivity objects.

This is a problem specific to spectral_connectivity_time(); spectral_connectivity_epochs() was already using the fixed approach.

@tsbinns
Copy link
Collaborator Author

tsbinns commented Mar 6, 2024

Currently the only method where spatial patterns are produced is MIC (not MIM or GC methods). I created a new variable _patterns_methods to specify those methods where the patterns array should be initialised:

_multivariate_methods = ["mic", "mim", "gc", "gc_tr"]
_gc_methods = ["gc", "gc_tr"]
_patterns_methods = ["mic"] # methods with spatial patterns

I could have hard-coded this, however I am just thinking ahead to when #163 is finished and the array should also be initialised for CaCoh.

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

Is there a unit-test that we can add to prevent this from occurring, or is this an internal detail?

@tsbinns
Copy link
Collaborator Author

tsbinns commented Mar 6, 2024

Is there a unit-test that we can add to prevent this from occurring, or is this an internal detail?

Personally, I would consider this more of a one-off oversight in the implementation from a mistake on my end.

Tbh I wouldn't know where to start in defining an appropriate memory usage for a function such as this.

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

SG! LGTM minus a small nit

Co-authored-by: Adam Li <adam2392@gmail.com>
@drammock drammock enabled auto-merge (squash) March 6, 2024 18:28
@drammock drammock merged commit f9a2e65 into mne-tools:main Mar 6, 2024
8 of 9 checks passed
@tsbinns tsbinns deleted the pr-fix_memory_patterns branch March 6, 2024 18:39
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