Skip to content

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
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