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

MNT: use brain_gc fixture in some tests #8427

Merged
merged 9 commits into from Oct 27, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Oct 26, 2020

This PR adds the brain_gc fixture in remaining 3d viz tests.

Closes #8403

I ran the tests locally using master versions of pyvista and pyvistaqt. Maybe some adjustments are necessary.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Oct 26, 2020
@@ -615,6 +616,7 @@ def test_plot_source_estimates(renderer_interactive, all_src_types_inv_evoked,
these_kwargs = kwargs.copy()
these_kwargs.pop('src')
meth(**these_kwargs)
renderer_interactive.backend._close_all()
Copy link
Member

Choose a reason for hiding this comment

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

Rather than do this, how about we just have both renderer_interactive and brain_gc call this after their yield steps? Calling it more than once should be fine, and having it in both means we shouldn't need to worry about order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll see if this solves the need to the intermediate calls too

@GuillaumeFavelier
Copy link
Contributor Author

There is indeed a difference between pinned and latest versions.

I'll come back to 908c1d2 when new versions are released. For now, some adjustments are necessary here.

@GuillaumeFavelier
Copy link
Contributor Author

I'll see if this solves the need to the intermediate calls too

For me, it did the trick locally, thanks 👍

Actually there is another strategy: waiting for a new release of pyvista before merging this. Since it's not high priority, I think we can afford to wait.

@larsoner
Copy link
Member

I would

  1. combine this PR and WIP: Use latest pyvista #8410
  2. make sure latest PyVista and PyVistaQt master are installed on CircleCI and the Travis (and Azure) pip pre runs
  3. triage the brain_gc to only check if new enough PyVista and PyVistaQt are installed

There is a a slight problem, though, that PyVista is currently at 0.26.0 for some reason. I'll open a PR to fix this, it's weird.

Comment on lines +487 to +488
import pyvista
if LooseVersion(pyvista.__version__) <= LooseVersion('0.26.1'):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import pyvista
if LooseVersion(pyvista.__version__) <= LooseVersion('0.26.1'):
if not check_version('pyvista', '0.26.1'):

Copy link
Member

Choose a reason for hiding this comment

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

... but things are green so let's just do this later. Thanks @GuillaumeFavelier !

@larsoner larsoner merged commit 92bcf29 into mne-tools:master Oct 27, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the maint/brain_gc_tests branch October 27, 2020 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use brain_gc in remaining tests
2 participants