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

ENH: Cache grand-average steps #765

Merged
merged 17 commits into from
Jul 19, 2023
Merged

ENH: Cache grand-average steps #765

merged 17 commits into from
Jul 19, 2023

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jul 17, 2023

Before merging …

  • Changelog has been updated (docs/source/changes.md)
  • Add caching of sensor grand average step
  • Add rerun timing test
  • Add caching of source grand average step
  • Check output HTMLs

I expect some failures because the source grand average step is not complete (these should fail the rerun test!), but this at least runs on ds003392 and ds004229 for me locally, and rerunning completes within 10s.

With this PR, I think all cachable steps are cached! Should make rerunning tests faster locally too 🚀

Closes #609

@larsoner
Copy link
Member Author

Okay I'll mark for merge-when-green once I work out the source step (that should still fail on current commit) and double check the outputs!

@larsoner
Copy link
Member Author

I'm going to change the STC filetype to .h5 so we don't have to worry about the annoying -lh.stc appending. In theory it should be a better format storage-wise anyway...

@larsoner
Copy link
Member Author

... and I noticed that our coverage was wrong for stuff like CSP time/freq -- I added some infrastructure to allow putting config overrides directly in mne_bids_pipeline/tests/test_run.py. That way they don't end up in the config.py files on the website, but they can do neat hidden stuff like set private stuff like _raw_split_size (it's no longer in the public config.py but still gets used!) and now a new _n_jobs that allows fine-grained setting of n_jobs based on the step. The latter allows us to set n_jobs=1 for specific steps where otherwise something like dask would be used, which allows coverage to be tracked properly for those steps rather than being lost.

So if I set this up properly our coverage of a file like _05_decoding_csp.py which is currently on codecov as ~16% (!) should shoot up to whatever it actually is based on its use in ERP_CORE_ERN, which was lost before because it was run in dask.

@larsoner
Copy link
Member Author

Hooray, coverage fixed!

Screenshot 2023-07-18 at 8 36 08 PM

I looked at the reports and just fixed the bug I found. I'll mark for merge when green 🤞

@larsoner larsoner marked this pull request as ready for review July 19, 2023 00:52
@larsoner larsoner enabled auto-merge (squash) July 19, 2023 00:52
@larsoner larsoner merged commit 81d6f55 into mne-tools:main Jul 19, 2023
5 checks passed
@larsoner larsoner mentioned this pull request Jul 17, 2023
14 tasks
@larsoner larsoner deleted the cache branch July 19, 2023 15:59
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.

ENH: Minor fixes to add
2 participants