Blackrock add summary of automatic data segmentation #1769

h-mayorquin · 2025-09-08T21:21:05Z

The summary of the gaps look like this:

h-mayorquin · 2025-09-08T21:57:35Z

neo/rawio/blackrockrawio.py

            # read nsx headers
            nsx_header_reader = self._nsx_header_reader[spec_version]
            self._nsx_basic_header[nsx_nb], self._nsx_ext_header[nsx_nb] = nsx_header_reader(nsx_nb)
+


This is to better document how the sampling frequency of each stream is calculated according to the spec.

h-mayorquin · 2025-09-08T21:58:23Z

neo/rawio/blackrockrawio.py

            ("timestamps", "uint64"),
            ("num_data_points", "uint32"),
-            ("samples", "int16", self._nsx_basic_header[nsx_nb]["channel_count"]),
+            ("samples", "int16", (self._nsx_basic_header[nsx_nb]["channel_count"],)),


This removes a warning from numpy, raw number as shapes are deprecate and will be removed:

FutureWarning: Passing (type, 1) or '1type' as a synonym of type is deprecated; in a future version of numpy, it will be understood as (type, (1,)) / '(1,)type'. npackets = int((filesize - offset) / np.dtype(ptp_dt).itemsize)

And based on our testing this change is also backward compatible or should we test this over a greater range of numpy versions than our current skinny IO tests?

zm711

Few initial comments/questions.

zm711 · 2025-09-10T18:11:24Z

neo/rawio/blackrockrawio.py

+            # E.g. it is 1 for 30_000, 3 for 10_000, etc
+            nsx_period = self._nsx_basic_header[nsx_nb]["period"]


Is this 30_000 true for all versions? I vaguely remember that different versions had different resolutions but maybe I'm wrong?

And seems like this was hardcoded the same way below before.

Yes, that's how the spec defines it and the history of the spec does not mention changes on that value.

Sounds good to me. I just couldn't remember.

Can confirm, the nominal base rate has always been 30 kHz, and this is the rate used by sample groups 5 and 6. Other sample groups are just sub-sampling this base rate (hopefully with anti-aliasing built in).

zm711 · 2025-09-10T18:13:21Z

neo/rawio/blackrockrawio.py

                        else:
                            t_start = timestamps / ts_res
-                            t_stop = max(t_stop, t_start + length / self.sig_sampling_rates[nsx_nb])
+                            t_stop = max(t_stop, t_start + length / self._nsx_sampling_frequency[nsx_nb])


can this be explained to me why would the length lead to being longer than the the t_stop itself? Maybe I need to read the whole reader to understand this, but if there is a two second explanation that would be great!

I think this is overly-defensive programming to say that t_stop should be larger than 0, check the whole block:

t_stop = 0.0 for nsx_nb in self.nsx_to_load: spec = self._nsx_spec[nsx_nb] if "timestamp_resolution" in self._nsx_basic_header[nsx_nb].dtype.names: ts_res = self._nsx_basic_header[nsx_nb]["timestamp_resolution"] elif spec == "2.1": ts_res = self._nsx_params[spec](nsx_nb)["timestamp_resolution"] else: ts_res = 30_000 period = self._nsx_basic_header[nsx_nb]["period"] sec_per_samp = period / 30_000 # Maybe 30_000 should be ['sample_resolution'] length = self.nsx_datas[nsx_nb][data_bl].shape[0] if self._nsx_data_header[nsx_nb] is None: t_start = 0.0 t_stop = max(t_stop, length / self._nsx_sampling_frequency[nsx_nb]) else: timestamps = self._nsx_data_header[nsx_nb][data_bl]["timestamp"] if hasattr(timestamps, "size") and timestamps.size == length: # FileSpec 3.0 with PTP -- use the per-sample timestamps t_start = timestamps[0] / ts_res t_stop = max(t_stop, timestamps[-1] / ts_res + sec_per_samp) else: t_start = timestamps / ts_res t_stop = max(t_stop, t_start + length / self._nsx_sampling_frequency[nsx_nb])

writing 0 directly would have been more readable than defining the variable though.

Yeah I agree because we define t_stop in all branches right? So we don't need to guard with an initial t_stop. That was definitely my confusion. We could always update that later. It's not crucial.

Let's do it now, we both already read and understood and we think this could be improved.

Forget about it, the loop above is properly sort of aligning the t_stops across the nsx segments.

So, yes, let's improve this later.

zm711 · 2025-09-10T18:14:03Z

neo/rawio/blackrockrawio.py

            ("timestamps", "uint64"),
            ("num_data_points", "uint32"),
-            ("samples", "int16", self._nsx_basic_header[nsx_nb]["channel_count"]),
+            ("samples", "int16", (self._nsx_basic_header[nsx_nb]["channel_count"],)),


And based on our testing this change is also backward compatible or should we test this over a greater range of numpy versions than our current skinny IO tests?

zm711 · 2025-09-10T18:15:56Z

neo/rawio/blackrockrawio.py

+        # We convert this indices to actual timestamps in seconds
+        raw_timestamps = struct_arr["timestamps"]
+        timestamps_sampling_rate = self._nsx_basic_header[nsx_nb]["timestamp_resolution"]  # clocks per sec uint64 or uint32
+        timestamps_in_seconds = raw_timestamps / timestamps_sampling_rate


Could this division lead to any issues uint/float should just be float right?

Good point, let me double-check the casting rules to see if nasty surprises could appear.

Yes, I think that the unsigned integers will be cast to float before dividing so I don't expect anything here, can you think on something?

No I didn't have anything in mind, but since we've dealt with so many random overflows lately especially on the Neo side with the changes in numpy 2.0 I'm just scared of a blind spot in array math :/

Good, yes, I think this is safe

zm711 · 2025-09-10T18:17:10Z

neo/rawio/blackrockrawio.py

+            segmentation_report_message += "+-----------------+-----------------------+-----------------------+\n"
+            warnings.warn(segmentation_report_message)
+
+        for seg_index, seg_start_index in enumerate(segment_start_indices):


this is a little unclear. why do you have index index for your naming. Could we make this naming a little clearer for the future :)

I agree this could be improved.

h-mayorquin · 2025-09-10T18:23:29Z

And based on our testing this change is also backward compatible or should we test this over a greater range of numpy versions than our current skinny IO tests?

I think that shapes should have always been specified as tuples and using a scalar was the odd case that they are now banning. We can check if that notation works for the smallest version of numpy that we support (which is?) if you want to be extra careful.

zm711 · 2025-09-10T18:40:47Z

And based on our testing this change is also backward compatible or should we test this over a greater range of numpy versions than our current skinny IO tests?

I think that shapes should have always been specified as tuples and using a scalar was the odd case that they are now banning. We can check if that notation works for the smallest version of numpy that we support (which is?) if you want to be extra careful.

It's probably fine. I'm just being overly cautious. We go down to 1.24, but we only test on 1.26 for the IOs. You're probably right that we were doing it incorrectly before. So switching to correct shouldn't lead to problems.

h-mayorquin · 2025-09-10T19:33:33Z

And based on our testing this change is also backward compatible or should we test this over a greater range of numpy versions than our current skinny IO tests?

I think that shapes should have always been specified as tuples and using a scalar was the odd case that they are now banning. We can check if that notation works for the smallest version of numpy that we support (which is?) if you want to be extra careful.

It's probably fine. I'm just being overly cautious. We go down to 1.24, but we only test on 1.26 for the IOs. You're probably right that we were doing it incorrectly before. So switching to correct shouldn't lead to problems.

Ok.

zm711 · 2025-09-10T22:23:13Z

neo/rawio/blackrockrawio.py

+        # This is read as an uint32 numpy scalar from the header so we transform it to python int
+        header_size = offset or int(self._nsx_basic_header[nsx_nb]["bytes_in_headers"])


Didn't we get in this discussion last time that this fails in the case that an offset of 0 is given :) Or did you change more logic? somewhere else?

No, I made the same mistake, dammit.

zm711 · 2025-09-11T11:56:38Z

@cboulay, if you have a moment you can feel free to read through this. I know you're plugged into a couple issues we are working through with blackrock, so I figure it's better to keep you in the loop. My plan is to get this merged by tomorrow as it is mostly helping us debug time gap issues for additional future work.

zm711 · 2025-09-14T15:21:14Z

Small delay, but this is on my list to do final review asap.

zm711 · 2025-09-15T17:51:28Z

After tests I will merge.

blackrock improved message

ad70604

h-mayorquin changed the title ~~blackrock improved message~~ Blackrock add summary of automatic data segmentation Sep 8, 2025

h-mayorquin mentioned this pull request Sep 8, 2025

BlackrockRawIO is over-segmenting data #1770

Open

improve erro furhter

b32247d

h-mayorquin commented Sep 8, 2025

View reviewed changes

h-mayorquin added 4 commits September 8, 2025 15:59

typo

33312ce

variable naming

a2ac0e2

anotehr typo

52155f8

another typo

967c541

h-mayorquin marked this pull request as ready for review September 9, 2025 03:00

Merge branch 'master' into add_informative_message_to_blackrock

79bafe9

h-mayorquin mentioned this pull request Sep 10, 2025

General API for handling sample gaps on rawio #1773

Open

zm711 reviewed Sep 10, 2025

View reviewed changes

readability

17d2baf

zm711 reviewed Sep 10, 2025

View reviewed changes

fix error verison II the revenge

5a3d227

Merge branch 'master' into add_informative_message_to_blackrock

8c3ac95

zm711 merged commit 72ec76a into NeuralEnsemble:master Sep 15, 2025
3 checks passed

zm711 added this to the 0.14.3 milestone Sep 15, 2025

zm711 mentioned this pull request Sep 15, 2025

Blackrock: improve nev header reading #1771

Merged

h-mayorquin deleted the add_informative_message_to_blackrock branch September 15, 2025 18:53

h-mayorquin mentioned this pull request Oct 8, 2025

Fix over-segmentation in BlackRock #1789

Open

		# E.g. it is 1 for 30_000, 3 for 10_000, etc
		nsx_period = self._nsx_basic_header[nsx_nb]["period"]

		# This is read as an uint32 numpy scalar from the header so we transform it to python int
		header_size = offset or int(self._nsx_basic_header[nsx_nb]["bytes_in_headers"])

Blackrock add summary of automatic data segmentation #1769

Blackrock add summary of automatic data segmentation #1769

Uh oh!

Conversation

h-mayorquin commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-mayorquin Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

h-mayorquin commented Sep 10, 2025

Uh oh!

zm711 commented Sep 10, 2025

Uh oh!

h-mayorquin commented Sep 10, 2025

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!

zm711 commented Sep 11, 2025

Uh oh!

zm711 commented Sep 14, 2025

Uh oh!

zm711 commented Sep 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

h-mayorquin commented Sep 8, 2025 •

edited

Loading

h-mayorquin Sep 8, 2025 •

edited

Loading