mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.72k stars 1.32k forks source link

BUG: 3D functions return non-public members #9961

Closed larsoner closed 2 years ago

larsoner commented 3 years ago

@GuillaumeFavelier we need a public equivalent to mayavi.core.api.Scene for whatever we currently return for create_3d_figure that users can pass around to 3D functions as fig=fig etc. Can you work on this separately? Then we can git grep "PyVista renderer" and know all the places we need to put in a reference that links properly.

adapted from https://github.com/mne-tools/mne-python/pull/9945#issue-772527626

larsoner commented 2 years ago

@GuillaumeFavelier this is marked for 1.0, can you take a look?

GuillaumeFavelier commented 2 years ago

I'll work on it next week :+1:

GuillaumeFavelier commented 2 years ago

I think the best candidate is the private _Figure:

https://github.com/mne-tools/mne-python/blob/main/mne/viz/backends/_pyvista.py#L46

I want to open a small PR to refactor _PyVistaRenderer first (mainly moving atrributes into _Figure).

Then we can think about the contract that this fig should fulfill. I believe it works as intended already ('saved figure' passed between the 3d functions) but it could be the time to make things right before making it public.

larsoner commented 2 years ago

Then we can think about the contract that this fig should fulfill.

I think the smallest API as possible is best. So maybe just that:

  1. It can be passed as a fig to MNE-Python functions, and probably also that
  2. The .plotter attribute is accessible, and is a PyVista plotter instance

WDYT?

GuillaumeFavelier commented 2 years ago

WDYT?

Perfect, that's exactly the purpose of _Figure.

larsoner commented 2 years ago

Perfect, that's exactly the purpose of _Figure.

Okay cool, I can make a PR to make this public unless you're working on it @GuillaumeFavelier

GuillaumeFavelier commented 2 years ago

You can go for it @larsoner, it's always possible to refactor things around later on