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
66 stars 34 forks source link

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

Closed tsbinns closed 7 months ago

tsbinns commented 7 months ago

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:

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: https://github.com/mne-tools/mne-connectivity/blob/4eb935f7cb86ea31081f34845f0e1b6ae4ffa1d5/doc/whats_new.rst?plain=1#L28

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

larsoner commented 7 months ago

@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 commented 7 months ago

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

larsoner commented 7 months ago

Thanks @tsbinns !