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

WIP: Improve _Brain memory footprint #8018

Merged
merged 7 commits into from
Jul 21, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jul 16, 2020

This PR follows #7927 (comment) and uses various solutions to decrease memory consumption:

  • Avoid duplication of hemi init meshes by re-using the vtkPolyData dataset
  • Avoid duplication of hemi data by re-using the scene vtkPolyData dataset
  • Use kwargs to avoid code duplication between mesh() and _mesh()
  • Reuse self.geo[hemi].nn to avoid unnecessary computation

Feel free to comment on this @larsoner

@GuillaumeFavelier
Copy link
Contributor Author

Also how many data can we have on a hemi? I would like to avoid duplication for that too so I'll go ahead with the naive solution next.

@GuillaumeFavelier
Copy link
Contributor Author

Waiting for the first CI report then I'll push again.

@larsoner
Copy link
Member

Store normals to avoid unnecessary computation and waste of memory

These should already live in self.geo[hemi].nn, if not then load_geometry should be updated to add it using mne.surface._compute_normals

Also how many data can we have on a hemi? I would like to avoid duplication for that too so I'll go ahead with the naive solution next.

Eventually we will want to support having multiple data overlaid, possibly with different colormaps (?), but we probably need to refactor quite a bit to support it anyway

self._hemi_meshes[h] = mesh
self._hemi_actors[h] = actor
actor = self._hemi_actors[h]
self._renderer.add_actor(actor)
Copy link
Member

Choose a reason for hiding this comment

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

Are you allowed in VTK to add the same actor to multiple scenes/views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, it works great for me but apparently this is not recommended...

Reference: http://vtk.1045678.n5.nabble.com/Sharing-vtkRenderer-td5735337.html

I have to rethink all that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example of the thread mentions multiple windows whereas in our case, it's multiple renderers of the same window. I'll rework that anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everywhere I look says that actors cannot be shared, the same goes for mappers. The only thing that can be shared is the PolyData itself. I'll restart from scratch.

@@ -1116,7 +1125,8 @@ def set_time_point(self, time_idx):
act_data = smooth_mat.dot(act_data)

# update the mesh scalar values
for mesh in hemi_data['mesh']:
if hemi_data['mesh'] is not None:
mesh = hemi_data['mesh']
if mesh is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This is now redundant

hide=False):
def _add_polydata(renderer, polydata, name=None,
hide=False):
if not hasattr(renderer, "plotter"):
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a comment about when this can happen?

@larsoner
Copy link
Member

larsoner commented Jul 16, 2020

Running mayavi on this PR (should be the same as master):

MNE_3D_BACKEND=mayavi BUILD_DEV_HTML=1 mprof run sphinx-build -D plot_gallery=1 -D sphinx_gallery_conf.filename_pattern=source_space_snr -b html -d _build/doctrees  -nWT --keep-going . _build/html

I get:

Screenshot from 2020-07-16 11-16-29

Adding time_viewer=False, show_traces=False (to make it more equivalent) and MNE_3D_BACKEND=pyvista I get something very similar:

Screenshot from 2020-07-16 11-22-10

On this PR it looks like memory is reduced (hooray!), but only by a few hundred MB (better than nothing):

Screenshot from 2020-07-16 11-18-30

Removing the time_viewer=False, show_traces=False (implicitly thus setting them to True) we then get the monster:

Screenshot from 2020-07-16 11-19-39

So it must be something about these options. Let's try show_traces=False, time_viewer=True (good!):

Screenshot from 2020-07-16 11-28-15

And then show_traces=True, time_viewer=False (also good, must be because show_traces has no effect when time_viewer=False):

Screenshot from 2020-07-16 11-29-47

@GuillaumeFavelier it seems that the problem is really having both of these enabled at once, which means it's something about how we do show_traces=True I wonder if it's the matplotlib callbacks or something... In any case, how about this plan:

  • You sort out the actor/data reuse in this PR, and we merge it (the few hundred MB savings are worth it already, and orthogonal to the memory usage issue)
  • we make a new PR to make show_traces=True, time_viewer=False raise an error (currently you have to have time_viewer=True to have show_traces actually work)
  • deal with the show_traces+time_viewer problem in another PR
  • I add a _MNE_BRAIN_TIME_VIEWER_AUTO=false to CircleCI that will be respected by _check_time_viewer_compatibility in the meantime -- it will allow us to get nice traces on our plots in MRG, ENH: Scrape traces when available #7927 while we work on the show_traces+time_viewer problem

@larsoner
Copy link
Member

Never mind that last point, it doesn't make any sense. I'll work on looking into the show_traces mem problem...

@larsoner
Copy link
Member

I think I found the circular ref, it was passing the times and act_data[vert_idx] to mpl_canvas.plot, which created a nasty circular reference. Passing times.copy() and act_data[vert_idx].copy() seems to fix things. Will push to #7927

@larsoner
Copy link
Member

Other huge memory blowup is actually storing self.act_data[hemi] = smooth_mat.dot(act_data), i.e., full-resolution data (!). We shouldn't need to do this and can just smooth_mat[idx].dot as needed

@agramfort
Copy link
Member

agramfort commented Jul 16, 2020 via email

@larsoner
Copy link
Member

While messing with this, I notice this in testing:

15.11s call     mne/viz/tests/test_3d.py::test_plot_alignment[pyvista]
8.61s call     mne/viz/tests/test_3d.py::test_plot_alignment[mayavi]

PyVista is much slower for plot_alignment. It's possible that this is because of the normal computation. We should be able to pass our own computed nn directly to renderer.mesh(..., normals=nn) to avoid this.

Basically I think every call to renderer.mesh(...) in our codebase should pass normals and (in PyVista) set smooth_shading=False. I'll add it to the todo, but we can remove if you disagree.

@larsoner larsoner added this to the 0.21 milestone Jul 16, 2020
@GuillaumeFavelier
Copy link
Contributor Author

I'll add it to the todo

It's a good idea. I mean if those infos are available, we should use them instead of re-calculating from scratch.

@larsoner
Copy link
Member

... and even if they're not available:

  1. We know that PyVista's computation via VTK can be problematic / cause crashes on some platforms
  2. We need to compute them for Mayavi anyway, so it reduces the number of code paths we need to consider by always computing and passing the normals directly to each renderer backend

@GuillaumeFavelier
Copy link
Contributor Author

In the current state, we're close to the goal but I'm not too sure about the memory gain anymore, I need to run mprof. Also smooth shading was already disabled by default for renderer.mesh().

At least we don't re-create a PolyData when possible and use the given normals data.

@larsoner
Copy link
Member

Also smooth shading was already disabled by default for renderer.mesh().

Yes, we passed smooth_shading=False to PyVista. This fixed some VTK segfault bug, but introduced a nasty visual regression in that the surfaces were not smooth shaded anymore.

In this PR, because we provide the normals directly/manually, the surfaces should be smooth shaded now. Are they?

@GuillaumeFavelier
Copy link
Contributor Author

When the normals parameter is passed to mesh(), the infos are used to allow Phong smooth shading.

@larsoner
Copy link
Member

Great, Phong is back! Thanks @GuillaumeFavelier

@larsoner larsoner merged commit 6db1b33 into mne-tools:master Jul 21, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the brain_improve_memory branch July 22, 2020 09:14
@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jul 31, 2020
28 tasks
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