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

[MAINT] Refactor bivariate and multivariate methods into separate files #156

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Nov 18, 2023

Sorry for the delay. As discussed in #127 and #142, the size of epochs.py (was 2,131 lines) has been made more manageable by splitting into 3 files:

  • epochs.py (1,098 lines) - handles code shared by all connectivity methods.
  • epochs_bivariate.py (358 lines) - handles bivariate connectivity estimator classes.
  • epochs_multivariate.py (749 lines) - handles multivariate connectivity estimator classes and multivariate connectivity helper functions.

As requested, this has only involved copy and pasting existing code and rearranging the imports.

For time.py (was 1,099 lines), the multivariate methods rely on the implementations that were in epochs.py, and the bivariate methods only take up ~100 lines, so there's not much to clean up.

I also updated whats_new.rst to reflect some missing information following the merge of #142:

- Add support for multivariate connectivity methods in :func:`mne_connectivity.spectral_connectivity_epochs` and :func:`mne_connectivity.spectral_connectivity_time` by `Thomas Binns`_ and `Tien Nguyen`_ and `Richard Köhler`_ (:pr:`138`) and (:pr:`142`).

Should these refactoring changes also be listed in the API changelog, or do we not mention this if they are all internal changes?

Also open to any comments/suggestions. Cheers!

@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 18, 2023

Majority of failing tests coming from FutureWarning when calling Epochs.get_data() following #12121 in MNE dev version:
FutureWarning: The current default of copy=False will change to copy=True in 1.7. Set the value of copy explicitly to avoid this warning

Tests using MNE stable version are passing.

Also an error from missing PyQt6.

@drammock
Copy link
Member

Should these refactoring changes also be listed in the API changelog

Normally we only mention refactorings in the changelog if they led to an API change

I can't easily review (only phone access at the moment) but the PR description sounds like what I was expecting. Thanks!

if it's obvious what test fixes are needed, could you do a quick separate PR for that (and then merge main here so CIs pass)? If not I can do it in about a week from now if nobody else gets there first

@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 21, 2023

Normally we only mention refactorings in the changelog if they led to an API change

Sure, make sense.


if it's obvious what test fixes are needed, could you do a quick separate PR for that (and then merge main here so CIs pass)?

Can do, I would just specify the copy parameter in the get_data() method calls. Quick question @drammock : should I set this to the current behaviour in 1.6 (copy=False) or set to the default value from 1.7 onwards (copy=True)?

image

https://mne.tools/stable/generated/mne.Epochs.html#mne.Epochs.get_data

@drammock
Copy link
Member

should I set this to the current behaviour in 1.6 (copy=False) or set to the default value from 1.7 onwards (copy=True)?

In tests use false whenever possible

@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 27, 2023

@drammock Just FYI, I fixed the failing tests upstream in #157 and merged the changes, so everything is passing now. Cheers!

@larsoner
Copy link
Member

@tsbinns just to double check this is just a bunch of copy-paste plus import adjustments, right? If so then +1 for merge from me

@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 27, 2023

@larsoner exactly, no changes to how any functions/classes work, just copying existing code and changing the imports as appropriate.

@larsoner larsoner merged commit c523fe0 into mne-tools:main Nov 27, 2023
15 checks passed
@larsoner
Copy link
Member

Thanks @tsbinns !

tsbinns added a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
…es (mne-tools#156)

* [REFACTOR] split bivar. and multivar. methods

* add missing info. in whats_new.rst
larsoner added a commit to larsoner/mne-connectivity that referenced this pull request Jan 9, 2024
* upstream/main: (56 commits)
  [CI] Fix CIs (mne-tools#164)
  Fix azure
  Fix CIs
  New dev for v0.7
  [RELEASE] V0.6 (mne-tools#162)
  Try azure again
  [CI] Fix azure (mne-tools#161)
  Add gitblame
  [MAINT] Run black, isort, ruff, and other auto-linters on entire package (mne-tools#159)
  [MAINT] Replace setup.py with pyproject.toml (mne-tools#160)
  [BUG] Fixed issue w/ different rank-indices length (mne-tools#158)
  [MAINT] Refactor bivariate and multivariate methods into separate files (mne-tools#156)
  [MAINT] Fix failing unit tests & update CI packages (mne-tools#157)
  fixed grammar mistake
  switched to array indices & added inline comments
  updated default non-zero rank tolerance
  removed redundant list creation
  removed redundant ignored word
  switched to masked indices for multivariate conn
  updated time
  ...
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