mne-tools / mne-python

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

BUG: Possible inefficient updates to Brain #8986

Closed larsoner closed 3 years ago

larsoner commented 3 years ago

One reason Brain is (probably) slower on CIs and example builds is that it does at least some unnecessary updates. For example on #8985 + #8975 if I add a mne.set_log_level('debug') in some test script, each time I tweak a clim or update a time point I get:

>>> brain.set_time_point(0)
Color mapping 'curv' with Greys colormap and range [-1, 2]
Color mapping 'data' with <class 'numpy.ndarray'> colormap and range [4.73, 14.18]
Color mapping 'curv' with Greys colormap and range [-1, 2]
Color mapping 'data' with <class 'numpy.ndarray'> colormap and range [4.73, 14.18]

So it's being mapped twice. And if I modify the _Brain code to have this:

    def _update(self):
        logger.debug('Update')

Just on init I see:

Update
Update
Update
Update
Update
Update

So we get 6 renderer updates. This last part might not actually matter since just having _update return immediately it's still ~4 seconds to open the window and exit.

So this is mostly an issue to remind us to track down and profile to see where time is being taken.

GuillaumeFavelier commented 3 years ago

We opted-out from auto_update so for this one, I would suggest to add an update parameter to the public functions that would default to True. With this solution, the internal calls can have update=False. What do you think?

larsoner commented 3 years ago

I think right now we don't have to worry about any public updates, and can just assume that if someone does something, update should be called. The real problem as I see it so far is that update is called more than once for a single "do something" command from the user's end. For example, set_time_point above causes a double recomputation of the overlay scalars/colormapping, even though it should only happen once.

With the changes from #8985 for example we could update a smoke test of set_time_point(0) to check the update count for example with:

with catch_logging('debug') as log:
    brain.set_time_point(0)
log = log.getvalue()
assert log.count('colormap and range') == 2, log  # one curv + one hemi data

and this would fail on main because the count would be 4.