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: Speed up import by caching indented params #8829

Merged
merged 9 commits into from Feb 7, 2021

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Feb 2, 2021

So far 940 ms to 560 ms on my system by caching the indented versions of our docdict. It says 325 sec of this is importing numpy for me so not a ton left to be gained. I think there might be some other gains to be made for example by nesting scipy imports, though, so I'm at least going to try that.

Closes #8826

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2021

I'm getting 1152ms vs. 557ms on my machine, so already half the time! 👍

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2021

Down to 533ms with the latest changes!

@larsoner larsoner changed the title WIP, ENH: Speed up import by caching indented params MRG, ENH: Speed up import by caching indented params Feb 2, 2021
@larsoner
Copy link
Member Author

larsoner commented Feb 2, 2021

I ended up nesting all scipy imports, and improving (I think) our utils/linalg.py code by using caches to store the functions, which should be fast enough for our cases. Ready for review/merge from my end

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2021

497ms! LGTM but what about failing CIs?

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2021

Also, this should really be mentioned (and highlighted) in the changelog! Cutting import time in half is 🔥!

@larsoner
Copy link
Member Author

larsoner commented Feb 2, 2021

If we (reasonably) assume anyone doing any analysis will import numpy we could claim the "MNE import overhead" decreased from 700 ms to 200 ms :)

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2021

True! It's 482ms for me, of which numpy uses 130ms, which leaves 352ms. On main its 1102ms, minus numpy that's 972ms. So that's almost a 3x speed increase! Super nice, thanks for implementing this idea so quickly @larsoner!

@cbrnr
Copy link
Contributor

cbrnr commented Feb 3, 2021

@larsoner there are still errors with test_module_nesting.

@drammock
Copy link
Member

drammock commented Feb 3, 2021

Also if we're nesting scipy now, there should be an update to the contributor guide about which imports to nest

@cbrnr
Copy link
Contributor

cbrnr commented Feb 3, 2021

Is the segfault legit?

@larsoner
Copy link
Member Author

larsoner commented Feb 3, 2021

Not relevant for this PR it's just #8828

@agramfort agramfort merged commit 0735446 into mne-tools:main Feb 7, 2021
2 checks passed
@agramfort
Copy link
Member

thx @larsoner

@larsoner larsoner deleted the import branch February 7, 2021 20:56
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.

MNE-Python import time
4 participants