Move Blackrock nsx handling closer to the buffer API pattern by h-mayorquin · Pull Request #1820 · NeuralEnsemble/python-neo

h-mayorquin · 2026-02-17T15:07:58Z

PR #1513 introduced BaseRawWithBufferApiIO, a base class that stores lightweight buffer descriptions (file paths, offsets, dtypes) instead of persistent np.memmap objects, creating short-lived mmap views on demand through cached file descriptors. This PR moves BlackrockRawIO toward that same pattern. We cannot fully adopt BaseRawWithBufferApiIO because: (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.ndarray with custom strides) 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.

zm711

Initial round of comments. I'm not as familiar with this reader so might need a bit more explanation in general @h-mayorquin.

zm711 · 2026-07-09T12:48:42Z

+            strides=strides,
+        )
+
+    def _get_nsx_fid(self, nsx_nb):


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?

You've got it right, it's lazy. On the first call it creates the fid dict, then for each nsx number it opens the file and caches the handle on the first miss, returning the cached handle on every subsequent call. So the handles are built up one at a time as they're first needed, not all at once.

I kept it lazy deliberately to mirror the buffer API's own file-handle pattern, where the multiplexed path opens each buffer's fid on demand, caches it, and reuses it on later calls rather than opening everything upfront. This is the same convention keyed by nsx number.

It also genuinely defers work here. Only PTP files open during header parsing; non-PTP files don't open until the first data read. So a reader that just inspects the header, or only reads one stream, never opens files it doesn't touch. The fail-fast case is already covered by the missing-file check during parsing, which validates the requested set exists before we reach this point.

zm711 · 2026-07-09T12:50:01Z

 """

 import datetime
+import mmap


the benefit of mmap vs np.memmap?

As mentioned in our in-person discussion this is the pattern that we use in the buffer array API and we use it for a reason: it behaves better with the python garbage collector.

I don't think either me or Sam understand that one that well but my guess is (with emphasis on guess) np.memmap slices keep references to the original memmap/file whereas np.ndarray(buffer=...) does not.

zm711 · 2026-07-09T12:52:01Z

-        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"]


Could we get a better variable name for readability?

Should be done.

zm711 · 2026-07-09T12:56:33Z

-        # Extract data and timestamps from structured array
-        data = file_memmap["samples"]
-        timestamps = file_memmap["timestamps"]
+        del temp_memmap


why are we deleting the temp_memmap twice?

no longer here.

zm711 · 2026-07-09T12:58:02Z

-                    "timestamp": timestamps,  # Use singular for backward compatibility
-                    "nb_data_points": data.shape[0],
+                segments.append({
+                    "timestamp": block_info["timestamps"],  # Use singular for backward compatibility


Do we know what this backward compatibility is?

Thanks for checking this.

I dug into this and there was no real backward compatibility being preserved, the comment was misleading. The value under that key has always been a single scalar (one timestamp per block), and the only thing consuming the plural "timestamps" key was the segmentation step, which immediately renamed it back to the singular "timestamp" that the rest of the reader already uses. It never crossed a module boundary or fed the per-sample timestamps path. So it was just an inconsistent name from the refactor, and I collapsed it to the singular key that matches the value's actual cardinality and dropped the comment.

zm711 · 2026-07-09T13:02:12Z

-                        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():


where is _key even being used?

Let's switch to values, this is not used.

zm711 · 2026-07-09T13:14:28Z


                # Check all nonempty nsX segments
-                for i, seg in enumerate(list_nonempty_nsx_segments[:]):
+                for i, seg in enumerate(nonempty_nsx_segments[:]):


If you're already changing names of stuff want to change i into something more interpretable for future us?

Hes. Good idea. Done.

zm711 · 2026-07-09T13:16:17Z

            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"]


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?

The original used .shape[0] because the old reader eagerly memmapped every segment into self.nsx_datas, so the array was already in hand and .shape[0] was the natural way to get the sample count. The buffer-API refactor doesn't eager-map anymore, it stores memmap_kwargs and maps on demand, so there's no materialized array here to measure, and reading the stored nb_data_points is the equivalent.

My thinking:
It can't drift from the actual shape, because both come from the same source: the block header's nb_data_points field for standard formats, the filesize computation for v2.1, or the PTP gap-segment length. The data view's shape is derived from that same number rather than measured independently, so they're equal by construction. On master it actually round-tripped, nb_data_points = data.shape[0] where the memmap was created with that same count, so .shape[0] was never independent ground truth either.

zm711 · 2026-07-09T13:17:44Z

-                    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):


Nice switch to reversed

zm711 · 2026-07-09T13:20:31Z

+            # 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


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.

Your intuition is exactly right. The old code had the inner j loop and decremented ev_ids[ev_ids == j] one index per iteration; the new code does it in a single vectorized ev_ids[ev_ids > seg_index] -= 1. The reason it can is that the segment container changed from a dict keyed by segment index to a list. On master, removing a segment meant manually re-keying every higher segment j -> j-1 in an inner loop, and the nev remap was interleaved into that loop, hence == j per pass. With a list, del [seg_index] shifts everything down automatically, so there's no inner loop and the nev ids above the removed segment can all be shifted at once. Same result either way: every id above the removed segment drops by one.

zm711 · 2026-07-09T14:03:04Z

-        file_memmap = np.memmap(filename, dtype=ptp_dt, shape=npackets, offset=header_size, mode="r")
+        ptp_dtype = np.dtype(ptp_dt)
+        npackets = int((filesize - header_size) / ptp_dtype.itemsize)
+        temp_memmap = np.memmap(filename, dtype=ptp_dt, shape=npackets, offset=header_size, mode="r")


Should this be changed to mmap?

Good point, and here, I was using this to check for ptp and I actually don't think we need this. So changing this.

h-mayorquin · 2026-07-13T20:58:50Z

All the comments should be addressed now @zm711 . Let me know if thereis anything else.

zm711 · 2026-07-13T23:29:42Z

Let me reread this tomorrow and then I'll merge barring anything I find.

Blackrock close to the buffer API pattern

2976dc1

h-mayorquin self-assigned this Feb 17, 2026

h-mayorquin changed the title ~~Blackrock nsx closer to the buffer API pattern~~ Move Blackrock nsx handling closer to the buffer API pattern Feb 17, 2026

remove death branch

79816fd

h-mayorquin requested a review from samuelgarcia February 17, 2026 15:30

alejoe91 added this to the 0.14.5 milestone Mar 25, 2026

alejoe91 modified the milestones: 0.14.5, 0.14.6 Jun 24, 2026

zm711 reviewed Jul 9, 2026

View reviewed changes

zm711 and others added 7 commits July 10, 2026 09:58

Merge branch 'master' into add_buffer_api_like_behavior_for_blackrock

54d7015

improve naming

93d9e1e

removing extra memmap

24d23ca

zach comments

51186cd

better naming

a5a013e

use values

a942641

Merge branch 'master' into add_buffer_api_like_behavior_for_blackrock

6ed525b

Merge branch 'master' into add_buffer_api_like_behavior_for_blackrock

68aaffa

Uh oh!

Conversation

h-mayorquin commented Feb 17, 2026

Uh oh!

zm711 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-mayorquin commented Jul 13, 2026

Uh oh!

zm711 commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants