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.

Sign up for GitHub

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

Jump to bottom

[MAINT] Fix failing unit tests & update CI packages #157

Merged
merged 14 commits into from
Nov 25, 2023
Merged

[MAINT] Fix failing unit tests & update CI packages #157

merged 14 commits into from
Nov 25, 2023

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Collaborator Author

tsbinns commented Nov 21, 2023

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
Copy link
Member

Sure I think that would be okay

@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 21, 2023

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
Copy link
Member

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
tsbinns commented Nov 22, 2023
View reviewed changes
.github/workflows/unit_tests.yml
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched unit tests with Python 3.8 -> 3.9, since the recent update to the minimum Python version for MNE was now causing these tests to fail.

@tsbinns tsbinns changed the title [MAINT] Specify data copy behaviour [MAINT] Fix failing unit tests & update CI packages Nov 22, 2023
@tsbinns
Copy link
Collaborator Author

tsbinns commented Nov 22, 2023

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

    mne-connectivity/requirements_doc.txt

    Lines 16 to 17 in 64e52f5

    PyQt6
    pyvistaqt

  2. added a pip install call in the config:

    mne-connectivity/.circleci/config.yml

    Lines 78 to 84 in 64e52f5

    name: Get Python running and install dependencies
    command: |
    pip install git+https://github.com/mne-tools/mne-python@main
    curl https://raw.githubusercontent.com/mne-tools/mne-python/main/tools/circleci_dependencies.sh -o circleci_dependencies.sh
    chmod +x circleci_dependencies.sh
    ./circleci_dependencies.sh
    pip install -r requirements_doc.txt

larsoner
larsoner approved these changes Nov 25, 2023
View reviewed changes
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.

Thanks @tsbinns !

@larsoner larsoner merged commit 43816ba into mne-tools:main Nov 25, 2023
13 checks passed
@tsbinns tsbinns deleted the pr-get_data_copy branch November 25, 2023 12:08
@tsbinns tsbinns mentioned this pull request Nov 27, 2023
Merged
tsbinns added a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
@tsbinns
[MAINT] Fix failing unit tests & update CI packages (mne-tools#157)
1b64fc1 
* [MAINT] Specify data copy behaviour

* [MAINT] Updated required MNE version

* Revert "[MAINT] Updated required MNE version"

This reverts commit bfb175e.

* [MAINT] Specify data copy behaviour for MNE>1.5

* [MAINT] Add missing PyQt6

* Revert "[MAINT] Add missing PyQt6"

This reverts commit 5836037.

* [MAINT] Add missing PyQt6 to CircleCI

* [MAINT] Add missing pyvistaqt to CircleCI

* [MAINT] Switch unit tests Python 3.8 to 3.9

* [MAINT] Add missing sphinx extension to CircleCI

* [MAINT] Add missing sphinx extension to CircleCI

* [MAINT] Add missing sphinx extensions to CircleCI

* [MAINT] Add missing sphinx extensions to CircleCI

* [MAINT] Add missing doc requirements to CircleCI
larsoner added a commit to larsoner/mne-connectivity that referenced this pull request Jan 9, 2024
@larsoner
Merge remote-tracking branch 'upstream/main' into deploy
8af7d36 
* 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
Reviewers

@larsoner larsoner larsoner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsbinns @larsoner