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

MRG, ENH: Scrape traces when available #7927

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jun 25, 2020

As @agramfort requested:

Screenshot from 2020-06-25 10-33-04

Todo:

  • update latest.inc to mention efficiency gains
  • move scraper to mne/viz/_brain/scraper.py
  • add show_traces=True, time_viewer=False error

@agramfort
Copy link
Member

Can you share a link to rendered doc ?

@larsoner
Copy link
Member Author

Actually it does not work on CircleCI because of _MNE_BRAIN_TRACES_AUTO=false :(

But it works fine locally!

@larsoner
Copy link
Member Author

I'm guessing that #7697 might allow us to remove _MNE_BRAIN_TRACES_AUTO=false

@GuillaumeFavelier
Copy link
Contributor

I'm guessing that #7697 might allow us to remove _MNE_BRAIN_TRACES_AUTO=false

We can try after it is merged but I expect some more memory optimization hacks to be necessary.

@larsoner
Copy link
Member Author

I expect some more memory optimization hacks to be necessary.

FWIW my expectation is that all of these problems probably come down to some processEvents needing to be called to properly pump Qt events to get all windows to close, garbage collection to happen at the right times and in the right orders, etc.

@larsoner larsoner changed the title MRG, ENH: Scrape traces when available WIP, ENH: Scrape traces when available Jun 25, 2020
@larsoner larsoner changed the title WIP, ENH: Scrape traces when available MRG, ENH: Scrape traces when available Jun 25, 2020
@larsoner larsoner changed the title MRG, ENH: Scrape traces when available WIP, ENH: Scrape traces when available Jun 25, 2020
@larsoner
Copy link
Member Author

Okay this does work:

But I still plan to more or less follow the order in #7791 before merging.

@agramfort
Copy link
Member

agramfort commented Jun 25, 2020

actually what I requested was to demo the white background but this is even better :)

@larsoner
Copy link
Member Author

larsoner commented Jun 25, 2020

Fixed the legend to use white-on-black text like the rest of the plot areas:

@agramfort
Copy link
Member

agramfort commented Jun 25, 2020 via email

@larsoner
Copy link
Member Author

Are the mat plot lib default colors adapted to this default black background?

No we manually set them (that was #7925). Arguably we should use some sort of style sheet or rcParams context. But that should be another PR to refactor how we do things, this PR just fixes it using the same methods in master.

@larsoner larsoner changed the title WIP, ENH: Scrape traces when available MRG, ENH: Scrape traces when available Jun 25, 2020
@GuillaumeFavelier
Copy link
Contributor

I added a small fix for style and started the CIs again.

@larsoner
Copy link
Member Author

When I run plot_mixed_source_space_inverse locally on my macos machine, it somehow uses 4 GB of memory. @GuillaumeFavelier have you used memprof to compare mayavi and PyVista? I won't have time today but can next week. It would be good to know if there is a big difference.

I'm the meantime I'll push a commit to skip just that example to see if it's the only remaining problem.

@larsoner
Copy link
Member Author

(I restarted the build probably five times yesterday and it always died on that example)

@GuillaumeFavelier
Copy link
Contributor

I also noticed a crash in test_plot_dipole_orientations:

https://travis-ci.org/github/mne-tools/mne-python/jobs/702324837#L3337

――――――――――――――――――――――――――― mne/viz/tests/test_3d.py ―――――――――――――――――――――――――――

[gw0] darwin -- Python 3.7.4 /Users/travis/miniconda/bin/python

worker 'gw0' crashed while running 'mne/viz/tests/test_3d.py::test_plot_dipole_orientations[pyvista]'

@GuillaumeFavelier
Copy link
Contributor

It's the Travis job on MacOS

@larsoner larsoner changed the title MRG, ENH: Scrape traces when available WIP, ENH: Scrape traces when available Jun 26, 2020
@larsoner
Copy link
Member Author

I also noticed a crash in test_plot_dipole_orientations:

I'm guessing that this is semi-spurious and related to one of the previous PRs we've merged rather than this one. I wonder if a processEvents style fix might also help there...

Let's see if CircleCI succeeds without the mixed_source_space_inverse example. If it does, I'll try reverting 0aa0174 3e7b239 92987a5 because they might just be unnecessary complications.

@larsoner
Copy link
Member Author

So on master this command, which uses mayavi:

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

produces a SG memory estimate of 1006 MB in 26 sec for the example, with mprof plot:

Figure_1

If we add a MNE_3D_BACKEND=pyvista, we get 1400 MB in 34 sec for the example, with mprof saying:

Figure_1

On this PR, SG estimates 1330 in 25 sec MB with mprof:

Figure_1

It looks like the last few builds died on plot_mixed_source_space_inverse and plot_resolution_metrics. I'll try disabling resolution_metrics, too, to see if that fixes things.

@larsoner larsoner force-pushed the brain_scraper branch 2 times, most recently from 09464d3 to f180b06 Compare July 14, 2020 19:26
@GuillaumeFavelier
Copy link
Contributor

I'm concerned about the 40% difference between the mayavi and pyvista backends. You think it's because of the traces?

I'll do some experiments locally and see if I can bring that down a little.

@larsoner
Copy link
Member Author

Not sure why that's happening. Maybe PyVista is computing some things for the meshes that it doesn't need to, or maybe it's making some copies of data somewhere? I'd start from a single hemi, single view, no data surface _Brain and PySurfer Brain and see what the memory usage is like. If it's already different there, then I'd start over from a simple mesh in plain Mayavi and PyVista and see if there is already a difference there.

@larsoner larsoner changed the title WIP, ENH: Scrape traces when available MRG, ENH: Scrape traces when available Jul 16, 2020
@larsoner
Copy link
Member Author

@GuillaumeFavelier pretty sure this should fix the memory problems:

Screenshot from 2020-07-16 12-28-27

Can you review? If you're happy I'll merge once CIs (including [circle full]) come back happy.

@larsoner
Copy link
Member Author

After this PR, it's possible that we can relax some of the slowtest and macOS skips. I have a feeling those might have been due to the whole-brain, while-time-course upsampling that is now avoided. But let's keep that separate so that we don't delay this further :)

Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier left a comment

Choose a reason for hiding this comment

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

This looks way better :)

My only nitpick is about the location of _BrainScraper, I would just move it into mne/viz/_brain/scraper.py for example. I know it makes everything verbose but that's where all the _Brain related things are. Or we can find a better plan in another PR.

@larsoner
Copy link
Member Author

Good call -- I'll try moving it there and updating latest.inc to mention the efficiency gains before merge. Don't want to push now since the circle full is running

@larsoner larsoner changed the title MRG, ENH: Scrape traces when available WIP, ENH: Scrape traces when available Jul 16, 2020
@larsoner
Copy link
Member Author

Added latest.inc, moved to _brain/_scraper.py, reorganized imports a bit, and made show_traces=True, time_viewer=False raise an error. I'll merge once CIs (other than notebook, which appears to be failing everywhere) come back happy!

@larsoner larsoner changed the title WIP, ENH: Scrape traces when available MRG, ENH: Scrape traces when available Jul 16, 2020
@larsoner larsoner merged commit bd6f426 into mne-tools:master Jul 16, 2020
@larsoner larsoner deleted the brain_scraper branch July 16, 2020 20:49
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