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

MRG: Prepare migration to PyVista 0.25 #7791

Merged
merged 56 commits into from
Jun 25, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented May 15, 2020

This PR tests the migration of BackgroundPlotter to https://github.com/pyvista/pyvista_qt

ToDo

Relevant to pyvista/pyvista#719

Closes #7697

@GuillaumeFavelier
Copy link
Contributor Author

Building the website frontpage is necessary too.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jun 5, 2020

pyvistaqt is on pypi! I can move forward with the migration

@GuillaumeFavelier
Copy link
Contributor Author

We have a few errors:

AttributeError: This plotter is closed and unable to save a screenshot.
UserWarning: VTK 9 no longer accepts an offset array

A crash:

―――――――――――――――――――――― mne/viz/_brain/tests/test_brain.py ――――――――――――――――――――――

[gw2] darwin -- Python 3.7.4 /Users/travis/miniconda/bin/python

worker 'gw2' crashed while running 'mne/viz/_brain/tests/test_brain.py::test_brain_timeviewer_traces[pyvista-both]'

A backend selection failure:

――――――――――――――――――――――――――――― test_get_3d_backend ――――――――――――――――――――――――――――――

[gw3] darwin -- Python 3.7.4 /Users/travis/miniconda/bin/python

mne/viz/backends/tests/test_renderer.py:162: in test_get_3d_backend

    assert renderer.get_3d_backend() == orig_backend

E   AssertionError: assert 'mayavi' == None

E    +  where 'mayavi' = <function get_3d_backend at 0x112db94d0>()

E    +    where <function get_3d_backend at 0x112db94d0> = <module 'mne.viz.backends.renderer' from '/Users/travis/build/mne-tools/mne-python/mne/viz/backends/renderer.py'>.get_3d_backend

@larsoner
Copy link
Member

larsoner commented Jun 5, 2020

Indeed the screenshot error is the one I got when I briefly tried to get it working. Not sure what the problem is there, but clearly just the from pyvistaqt import ... is not equivalent to what used to be done in pyvista :(

@GuillaumeFavelier
Copy link
Contributor Author

Let's try with BUILDING_GALLERY = True

Reference: pyvista/pyvista#783

@GuillaumeFavelier
Copy link
Contributor Author

I decided to change my approach. PyVista's architecture has changed a lot recently and this flow of issues is too much for me to handle at the same time. I will proceed from the latest working version, iterate one step at a time and adapt MNE to digest the changes.

@GuillaumeFavelier
Copy link
Contributor Author

Good start. master(78bc2a4) with 0.24.2 makes all the CIs happy and looking particularly at Azure here, I think it could be nice to pin to 0.24.2 instead of 0.24.0 as a short term solution @larsoner

@GuillaumeFavelier GuillaumeFavelier changed the title TST: Prepare BackgroundPlotter migration TST: Prepare migration to PyVista 0.25 Jun 14, 2020
@GuillaumeFavelier
Copy link
Contributor Author

Azure seems a tad slower but all good with 0.24.3 too, good news.

@agramfort
Copy link
Member

agramfort commented Jun 14, 2020 via email

@larsoner
Copy link
Member

😭

@larsoner
Copy link
Member

(I speak for the CIs)

@GuillaumeFavelier
Copy link
Contributor Author

Not everything is green, it's not failed tests but timeouts.

@GuillaumeFavelier
Copy link
Contributor Author

The render window is cleared with _clear_ren_win() before the screenshot, that's why we get:

AttributeError: This plotter is closed and unable to save a screenshot.

@GuillaumeFavelier
Copy link
Contributor Author

Migrating completely to BackgroundPlotter should fix this one (work in progress in #7697).
Those PRs are oddly connected.

@larsoner
Copy link
Member

Feel free to combine into a single PR if you think it would help

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jun 16, 2020

And this one:

UserWarning: VTK 9 no longer accepts an offset array

has been fixed upstream normally (around pyvista 0.25.1)

@larsoner
Copy link
Member

On my macOS machine I changed environment.yml to have python<3.8, then ran:

conda env create -n pyvistaqt environment.yml
conda activate pyvistaqt
cd mne/viz/_brain/tests
jupyter notebook

Then ran stc.py, it worked. Jupyter notebook had a "unexpected keyword argument 'io_loop'" that I then fixed with pip install tornado==5.4.3. Then pip install pyimpl (probably should have used server_environment.yml. Then segfault in _sphere->_add_mesh->pyvista/plotting/plotting.py->core/filters.py line 3001 in compute_normals. So I think this is probably related to things not being properly initialized at the VTK end.

@GuillaumeFavelier
Copy link
Contributor Author

Many thanks! Your detailed report gives me some more material to work with to debug the situation.

Should we disable smooth_shading then as a short term solution?

@GuillaumeFavelier
Copy link
Contributor Author

Disabling for now to move forward. We can always enable it later once the situation is clarified.

@larsoner
Copy link
Member

Yes, that at least fixes test_notebook.py at my end!

@larsoner
Copy link
Member

Sadly, this fixes the segfault but the plots do not show brains (colorbars, etc. show up but not the brains) for me in the notebook

@GuillaumeFavelier
Copy link
Contributor Author

Any error log by any chance? What script did you use?

@larsoner
Copy link
Member

Never mind, actually I was testing in the wrong environment! Things work locally for me on macOS now so +1 for merge

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Prepare migration to PyVista 0.25 MRG: Prepare migration to PyVista 0.25 Jun 25, 2020
@GuillaumeFavelier
Copy link
Contributor Author

Is it working for you too @agramfort?

@larsoner
Copy link
Member

@GuillaumeFavelier pushed 8be2205 to DRY our processEvents calls.

@GuillaumeFavelier
Copy link
Contributor Author

Wow, now it's packed with processEvents(). Fingers crossed for #7927

@larsoner
Copy link
Member

@GuillaumeFavelier my plan is to merge this pending @agramfort's approval, then #7931, then #7775, then #7927 (I'm about to enable traces there), probably with a merge or rebase at each step, sound good?

@agramfort
Copy link
Member

agramfort commented Jun 25, 2020 via email

@GuillaumeFavelier
Copy link
Contributor Author

Sounds like a plan

@agramfort agramfort merged commit 9800cf3 into mne-tools:master Jun 25, 2020
@agramfort
Copy link
Member

works like a charm !

awesome work @GuillaumeFavelier and @larsoner !

@GuillaumeFavelier GuillaumeFavelier deleted the pyvista_qt_migration branch June 25, 2020 20:40
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 25, 2020
* upstream/master:
  MRG: Prepare migration to PyVista 0.25 (mne-tools#7791)
  MAINT: Simpler VTK [circle front] (mne-tools#7931)
  MRG, ENH: Add arbitrary connectivity for stats (mne-tools#7916)
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 25, 2020
* upstream/master:
  MRG, ENH: Automatically fix magnetometers when maxwell filtering (mne-tools#7929)
  MRG: Prepare migration to PyVista 0.25 (mne-tools#7791)
  MAINT: Simpler VTK [circle front] (mne-tools#7931)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants