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

[MRG] Adding tests for viz. #27

Closed adam2392 closed 3 years ago

adam2392 commented 3 years ago

PR Description

Adds tests for viz that weren't added before.

Moved back circular_layout to mne.viz according to @larsoner comments.

Merge checklist

Maintainer, please confirm the following before merging:

britta-wstnr commented 3 years ago

If we want to follow MNE-Python logic, these should live in mne_connectivity/viz/tests and not mne_connectivity/tests/viz.

adam2392 commented 3 years ago

If we want to follow MNE-Python logic, these should live in mne_connectivity/viz/tests and not mne_connectivity/tests/viz.

Done.

adam2392 commented 3 years ago

Not sure why the GH actions were not running to completion, so just restarted the jobs.

adam2392 commented 3 years ago

@larsoner actually it looks almost like the entire unit testing suite is stalling on the GH Ubuntu build. Is there anything special with the conftest that I'm missing?

My suspicion was perhaps the plots are not closing inside the GH actions, which makes them time out?

larsoner commented 3 years ago

Oh yes you'll need the other viz fixture in MNE-Python that sets the backend to agg so that nothing blocks (the Qt backend, which is default on most OSes, will block):

https://github.com/mne-tools/mne-python/blob/main/mne/conftest.py#L170-L210

You shouldn't need the traits stuff but the exception handling is useful

adam2392 commented 3 years ago

Hmmm the viz is still stalling now without the explicit plt.close()?

larsoner commented 3 years ago

@adam2392 FYI it's better practice to open PRs from your fork rather than the main repo

adam2392 commented 3 years ago

Tbh, I'm not sure why things are still stalling:

Did I miss something?

larsoner commented 3 years ago

I think VTK 9.0.3 is the problem, we don't actually test it over in MNE-Python. For all builds we use conda except:

  1. The Linux 3.9 pre one , and for that we install a 9.0.1dev wheel that I compiled for Linux
  2. The Windows 3.8 pre where it installs 9.0.1

I pushed a commit that might fix it...

larsoner commented 3 years ago

It's a bit annoying that the GH actions are taking forever, so I added auto-cancelling via concurrency (see https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#concurrency)

larsoner commented 3 years ago
  1. Added pytest-timeout to hopefully help with timeouts
  2. Added pytest_configure to run matplotlib_config automagically
  3. Added warnings-as-errors
  4. Cast to real in VAR code to avoid a warning about that being done (comes from the sqrtm call)

@adam2392 can you see if this all makes sense to you?

larsoner commented 3 years ago

Bumped to Python 3.7+ requirement (same as MNE) and got rid of 3.6 on CIs

adam2392 commented 3 years ago

Ah nice, was the issue then coming from the fact that it was Python3.6?, or that Python3.9 was too high for certain vtk versions?

Or was it that I didn't set the matplotlib_config correctly before?

The LOC makes sense, but jw for my own sake.

larsoner commented 3 years ago

Or was it that I didn't set the matplotlib_config correctly before?

matplotlib_config was not being called because it wasn't being set to being automatically used (now it is by pytest_configure), and hence it was getting stuck when a matplotlib figure was created and shown.

The VTK stuff probably does not matter, I'll try setting that back.

adam2392 commented 3 years ago

Thanks @larsoner !