Repository navigation
Blackrock: read headers through an open file instead of np.fromfile and np.memmap - #1940
h-mayorquin wants to merge 2 commits into
Conversation
cboulay
left a comment
cboulay
left a comment
There was a problem hiding this comment.
Thanks Heberto, this is a good step and I'd be happy to see it merged. I checked it against master on all 53 Blackrock datasets in ephy_testing_data, with every nsX loaded. Headers are byte-identical, and segments, signal checksums, spikes and events match everywhere. The only change is the error text on a truncated header.
The performance win is bigger than the description suggests. The extended header is no longer an np.memmap, so the per-channel loop in _parse_header stops going through memmap.__getitem__, and parse_header gets about 4x faster (512-channel ns5: 212 → 52 ms; many_short_blocks: 4.7 → 1.2 s).
For #1931, I'd suggest one change, before or after merging: read through the handles the reader already caches (self._get_nsx_fid(nsx_nb), plus a matching _get_nev_fid()) rather than open(filename) at each site.
_read_nsx_dataheaderis called once per data block, so a file with a few hundred pauses opens the file a few hundred times. That costs nothing locally but a round trip each over fsspec.- With cached handles, supporting another file opener only means changing those two getters.
- A small helper,
read_records(fid, dtype, offset, count), that raises if it gets fewer bytes than asked would also remove the repeatednp.frombuffer(fid.read(n * itemsize), ...)and give a clearer error on truncated files.
We have both in our fork (a rawio utils helper and 7 call sites), and I'm happy to push them here or open a follow-up, whichever you prefer.
| return None # v2.1 has no data headers | ||
|
|
||
| nsx_basic_header = np.memmap(filename, dtype=data_header_dtype, shape=1, offset=offset, mode="r")[0] | ||
| with open(filename, "rb") as fid: |
There was a problem hiding this comment.
This one runs once per data block from _parse_nsx_data_v22_v30, so it opens the file once per block. Could it use fid = self._get_nsx_fid(nsx_nb) like the PTP parser a few hundred lines down?
| # Get basic header structure for this spec | ||
| basic_header_dtype = NSX_BASIC_HEADER_TYPES[spec] | ||
| nsx_basic_header = np.fromfile(filename, count=1, dtype=basic_header_dtype)[0] | ||
| with open(filename, "rb") as fid: |
There was a problem hiding this comment.
Same here and at 986/1047: _get_nsx_fid(nsx_nb) is already available, and the basic and extended header could share one handle.
| ] | ||
|
|
||
| nev_basic_header = np.fromfile(filename, count=1, dtype=dt0)[0] | ||
| with open(filename, "rb") as fid: |
There was a problem hiding this comment.
For the NEV, a _get_nev_fid() mirroring _get_nsx_fid would let 1006, 1663 and 1672 share a handle, and the NEV data read could use it later.
| raw_ext_header = np.memmap(filename, offset=offset_dt0, dtype=dt1, shape=shape, mode="r") | ||
| with open(filename, "rb") as fid: | ||
| fid.seek(offset_dt0) | ||
| raw_ext_header = np.frombuffer(fid.read(int(shape) * np.dtype(dt1).itemsize), dtype=dt1, count=int(shape)) |
There was a problem hiding this comment.
Nit: np.frombuffer on a short read gives "buffer is smaller than requested size". Checking len(data) and naming the offset and expected size would make truncated files easier to diagnose.
There was a problem hiding this comment.
I will address this on the common PR reader tooling where we can test it.
… cleanup and drop them when pickling
|
It is a nice surprise that the improvements are better than I thought for a change. I am keeping the file handles open now instead of re-opening as you requested. This is a pattern that we use already on the buffer API and more or less for the same reasons (we have discussed this before). The problem with keeping the file handles open is that it introduces possible leaks of open files so to eliminate this somehow I introduced explicit closing machinery with the dunder method Regarding the small utility, I thought about it while doing this PR but blackrock is not the only reader that uses this and I was thinking about doing a small PR after this with that change. Right now, all the instances are inlined here. |
|
I found another error: After you approve this and we merge it, I will take care of that one. |
I am thinking about #1931 and this is a low-hanging fruit to: 1) improve performance (even if it does not matter at this scale) and 2) make the header parsing more remote friendly.
The idea is that
np.fromfileandnp.memmaptake the file path and only work with a local file, and each call opens or maps the file again. Reading withseekandreadon an open file works with any file object and then the header parsing does not need to change if the file is opened another way (for example with fsspec).