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

BUG: Possible inefficient updates to Brain #8986

Closed
larsoner opened this issue Mar 3, 2021 · 2 comments · Fixed by #9407
Closed

BUG: Possible inefficient updates to Brain #8986

larsoner opened this issue Mar 3, 2021 · 2 comments · Fixed by #9407
Labels

Comments

@larsoner
Copy link
Member

larsoner commented Mar 3, 2021

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.

@larsoner larsoner added the BUG label Mar 3, 2021
@GuillaumeFavelier
Copy link
Contributor

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
Copy link
Member Author

larsoner commented Mar 3, 2021

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.

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 a pull request may close this issue.

2 participants