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: Add brain_gc fixture to test_process_clim_plot #8575

Merged
merged 2 commits into from
Nov 26, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

This PR adds the brain_gc fixture in test_process_clim_plot (I missed it in #8427) and fixes the issues with memory:

_______________________________________________________________________________ ERROR at teardown of test_process_clim_plot[pyvista] _______________________________________________________________________________
mne/conftest.py:522: in brain_gc
    assert len(bad) == 0, 'VTK objects linger:\n' + '\n'.join(bad)
E   AssertionError: VTK objects linger:
E     vtkXOpenGLRenderWindow
E     vtkGenericRenderWindowInteractor
E   assert 2 == 0
E     +2
E     -0

It's also important for #8335 (that's how I discovered the issue in the first place)

@GuillaumeFavelier GuillaumeFavelier self-assigned this Nov 26, 2020
Comment on lines 396 to 399
# figure has been set so the internal ref needs to be clean to avoid
# lingering VTK objects in memory
if is_pyvista:
renderer_interactive.backend._FIGURES.clear()
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand -- doesn't this indicate that the close_all that happens at the end of this test is somehow not working? How do we get a figure set/created in a way that our close-all fixture will not close it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep an internal reference to mimic mayavi's behaviour when figure (int) is used.

Copy link
Member

Choose a reason for hiding this comment

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

See follow-up comment -- I think this means close_all needs to be updated, or that we need to update our fixture to call another close_all for the case of int-passing or something.

@larsoner
Copy link
Member

To put it differently, whenever you add brain_gc to a test, any failures should be fixed in _brain.py, not in the test itself -- this is because brain_gc does a close_all sort of operation, and this should close all figures. If it doesn't, it suggests some problem with our close_all that should be fixed.

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.

LGTM feel free to merge once CIs are green @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier merged commit 606e88f into mne-tools:master Nov 26, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the fix/memory branch November 26, 2020 15:36
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.

3 participants