search

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] Fix failing unit tests & update CI packages #157

Closed tsbinns closed 7 months ago

tsbinns commented 7 months ago

Following #12121 in mne-python, the dev. version of MNE, the behaviour of the Epochs.get_data() method has been changed such that in v1.7, a copy of the data will be returned by default (rather than the current view).

Without specifying the copy parameter, a FutureWarning is raised: 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

This is causing several of the CI tests to fail when using the MNE dev. version.

Changes

Explicitly set copy=False in all instances where Epochs.get_data() is called. As I understand it the returned data is not modified itself, so returning a view should be sufficient.

N.B.: Since the copy parameter was added in MNE v1.6, we need to update the MNE version in requirements.txt to >= 1.6.

larsoner commented 7 months ago

N.B.: Since the copy parameter was added in MNE v1.6, we need to update the MNE version in requirements.txt to >= 1.6.

It just came out yesterday so this is a bit tough. Probably better to use inspect.getfullargspec like

https://github.com/mne-tools/mne-bids/blob/a3ca0c72f3c529ad66d9b85eea3b87a039fc3419/mne_bids/tests/test_write.py#L629

tsbinns commented 7 months ago

Checks still failing due to separate missing PyQt6: ModuleNotFoundError: No module named 'PyQt6' https://app.circleci.com/pipelines/github/mne-tools/mne-connectivity/786/workflows/4d35a381-b62a-4808-a298-b3b762f021c9/jobs/850/parallel-runs/0/steps/0-116

Should this be added to requirements_doc.txt?

larsoner commented 7 months ago

Sure I think that would be okay

tsbinns commented 7 months ago

Sorry if this is very basic, I'm not familiar with CI stuff.

PyQt6 is still not installed for the build_docs job.

I see that this file is called to install the dependicies: curl https://raw.githubusercontent.com/mne-tools/mne-python/main/tools/circleci_dependencies.sh -o circleci_dependencies.sh

Should the PyQt6 dependency be added here? Or, should a doc extra be provided for MNE-Connectivity? WARNING: mne-connectivity 0.6.0.dev0 does not provide the extra 'doc'

larsoner commented 7 months ago

To keep it simple I would just modify the CircleCI code here to pip install PyQt6 in some existing pip install line. Could be for example pip install PyQt6 -e. or whatever

tsbinns commented 7 months ago

Update

Wasn't just PyQt6 that was missing from CircleCI, pyvistaqt as well as several packages in requirements_doc.txt were not being installed.

Rather than list everything again in the CircleCI config, I:

  1. added PyQt6 and pyvistaqt to requirements_doc.txt; and https://github.com/mne-tools/mne-connectivity/blob/64e52f55ed0dbd352bc7239e19c838967ce48c49/requirements_doc.txt#L16-L17

  2. added a pip install call in the config: https://github.com/mne-tools/mne-connectivity/blob/64e52f55ed0dbd352bc7239e19c838967ce48c49/.circleci/config.yml#L78-L84