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: Speed up reading of small buffers #12343

Merged
merged 6 commits into from
Jan 10, 2024
Merged

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jan 9, 2024

Closes #12234

@britta-wstnr at the end of the day I don't think the numpy.dtype I considered in #12234 will be tractable/worth it as:

  1. there can be corrupt files (uncommon but do exist) and we can only find them by actually trying to read each buffer
  2. there can be acquisition skips and these need to be handled properly (currentlny easily handled in a loop... not easily if we try to vectorize)
  3. the single-sample case is pretty rare and sounds like there should be a fix at the acq end soon (?)
  4. there is a nice workaround of reading the files once with MNE-Python then writing them out with buffer_size_sec=1., then the I/O is no longer slow

Okay with you to close the associated issue when this is merged?

In the meantime this PR cleans up and speeds up our tag code, on my system it:

  1. Speeds up the single-sample-buffer snippet from ENH: Speed up FIF IO #12234 by a factor of about 2
  2. Speeds up pytest mne/io/fiff from 23.79s to 20.46s
  3. Simplifies the Tag logic and functions, especially involving when to fid.seek to a new position in a file (it's explicit now rather than implicit)
  4. Removes a completely unused copy_tree which is already in a private namespace (mne._fiff) so no need to deprecate, can just remove
  5. Prefers using explicit file positions to using fid.tell because this function was surprisingly a source of slowdowns in the single-sample buffer case

@larsoner larsoner added this to the 1.7 milestone Jan 9, 2024
mne/_fiff/open.py Outdated Show resolved Hide resolved
mne/_fiff/open.py Outdated Show resolved Hide resolved
mne/_fiff/tag.py Outdated Show resolved Hide resolved
lines = show_fiff(str(bad_fname), output=list)
last_line = lines[-1]
assert ">>>>BAD" in last_line
assert "302 = FIFF_EPOCH (734412b >f4)" in last_line
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch on the order above @drammock, this line used to be:

302  = FIFF_EPOCH (734412b ?1073741828?) >>>>>>>>>>>>>>>>>>>>BAD'

which is wrong, now it's correct:

302  = FIFF_EPOCH (734412b >f4) >>>>>>>>>>>>>>>>>>>>BAD

@drammock drammock enabled auto-merge (squash) January 10, 2024 16:49
@drammock drammock merged commit 16c17b4 into mne-tools:main Jan 10, 2024
29 checks passed
@drammock drammock deleted the efficient branch January 10, 2024 17:47
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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: Speed up FIF IO
2 participants