Move Blackrock nsx handling closer to the buffer API pattern#1820
Move Blackrock nsx handling closer to the buffer API pattern#1820h-mayorquin wants to merge 2 commits into
Conversation
h-mayorquin
changed the title
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
Initial round of comments. I'm not as familiar with this reader so might need a bit more explanation in general @h-mayorquin.
| strides=strides, | ||
| ) | ||
|
|
||
| def _get_nsx_fid(self, nsx_nb): |
There was a problem hiding this comment.
Could you walk me through how this construction is going to happen? Shouldn't we grab all nsx_fids at one time to construct them. This calls and caches the fids at it goes right? creating the dict on the first call?
| """ | ||
|
|
||
| import datetime | ||
| import mmap |
There was a problem hiding this comment.
the benefit of mmap vs np.memmap?
| memmap_data = self.nsx_datas[nsx_nb][seg_index] | ||
| seg = self._nsx_data_header[nsx_nb][seg_index] | ||
| fid = self._get_nsx_fid(nsx_nb) | ||
| kw = seg["memmap_kwargs"] |
There was a problem hiding this comment.
Could we get a better variable name for readability?
| # Extract data and timestamps from structured array | ||
| data = file_memmap["samples"] | ||
| timestamps = file_memmap["timestamps"] | ||
| del temp_memmap |
There was a problem hiding this comment.
why are we deleting the temp_memmap twice?
| "timestamp": timestamps, # Use singular for backward compatibility | ||
| "nb_data_points": data.shape[0], | ||
| segments.append({ | ||
| "timestamp": block_info["timestamps"], # Use singular for backward compatibility |
There was a problem hiding this comment.
Do we know what this backward compatibility is?
| ev_ids[ev_ids == j] -= 1 | ||
| # Remap nev segment ids: shift down all ids above the removed segment | ||
| if self._avail_files["nev"]: | ||
| for _key, (data, ev_ids) in self.nev_data.items(): |
There was a problem hiding this comment.
where is _key even being used?
|
|
||
| # Check all nonempty nsX segments | ||
| for i, seg in enumerate(list_nonempty_nsx_segments[:]): | ||
| for i, seg in enumerate(nonempty_nsx_segments[:]): |
There was a problem hiding this comment.
If you're already changing names of stuff want to change i into something more interpretable for future us?
| keep_seg = True | ||
| for nsx_nb in self.nsx_to_load: | ||
| length = self.nsx_datas[nsx_nb][data_bl].shape[0] | ||
| length = self._nsx_data_header[nsx_nb][seg_index]["nb_data_points"] |
There was a problem hiding this comment.
Something I keep seeing. Why didn't the original implementation use this nb_data_points value? It seems to have been present before. They seem to want to use the shape of the data. Is it possible that the dict value is ever off from the actual data shape?
| data_header = self._nsx_data_header[nsx_nb].pop(j) | ||
| self._nsx_data_header[nsx_nb][j - 1] = data_header | ||
| # Remove empty segments in reverse order to preserve indices | ||
| for seg_index in reversed(empty_indices): |
| # Remap nev segment ids: shift down all ids above the removed segment | ||
| if self._avail_files["nev"]: | ||
| for _key, (data, ev_ids) in self.nev_data.items(): | ||
| ev_ids[ev_ids > seg_index] -= 1 |
There was a problem hiding this comment.
This was the other bit I was getting a lost at. So you check for > seg_index, but they seem to be only checking that j is equal. So let me double check that I understand correctly. Previously they were doing a nested loop with the j and the ev_ids and then basically it would iterate one value per loop. What you are doing is doing only one loop through the data and changing all applicable values during that one loop, right? If that's not the correct intuition than I might need you to explain this bit to me.
3 participants
PR #1513 introduced
BaseRawWithBufferApiIO, a base class that stores lightweight buffer descriptions (file paths, offsets, dtypes) instead of persistentnp.memmapobjects, creating short-lived mmap views on demand through cached file descriptors. This PR moves BlackrockRawIO toward that same pattern. We cannot fully adoptBaseRawWithBufferApiIObecause: (a) Blackrock's standard format (v2.2/2.3/3.0) stores multiple data blocks with explicit headers in a single file, requiring block-aware offset tracking that the base class's one-buffer-per-stream model does not support; (b) the PTP variant interleaves per-sample timestamps with sample data in fixed-size packets, requiring strided mmap access (np.ndarraywith customstrides) rather than contiguous views; and (c) the segmentation logic for PTP files needs to read timestamps from the file to detect gaps, which couples parsing and segmentation in a way the base class does not allow. However, we can adopt the core idea: replace stored memmaps with on-demand creation.This has the added benefit of reducing the number of memmaps that are open at any time and we have seen that this pattern is both 1) more performant memory wise, 2) less likely to lead to leaks. Importantly, this PR does not change any behavior of the reader, it is an internal optimization only. It lays the groundwork for a follow-up PR adding gap detection on the standard format.