Minor misuse of timing info in Blackrock headers

[cboulay]

Describe the bug

There are a couple instances in blackrockrawio.py where the information about timestamp resolution and sample resolution are misused. So far this has proven innocuous because the resulting variables are unused, and even if they were used the mistake happens to yield the same value, for now.

That's about to change. There is an upcoming change to Blackrock files in the timestamp resolution (30_000/s -> 1e9/s, i.e., nanoseconds); sample resolution remains unchanged (30_000/s). Technically, this does not require a FileSpec version change because all of the required information is already in the existing FileSpec; it just needs to be used correctly. If NEO remains unchanged, its misuse of the header information will lead to incorrect entries in self._nsx_params, which are currently unused, but we should correct them anyway in case they are ever used in the future.

Note: The NEV header has both timestamp resolution (current: 30_000 -> new: 1e9) and sample resolution (30_000 -> 30_000). The NSx header only has timestamp resolution (30_000 -> 1e9) and, as per the filespec, we are expected to assume that the NSx sample resolution is 30_000. If the NSX header is modified in the future to include sample resolution then this would be an explicit change to the file spec; for the new file spec we would replace the hard-coded 30_000 with the header-provided value.

To Reproduce
The FileSpec doc is attached LB-0023-7.00_NEV_File_Format.pdf. We can discuss if the proposed changes are feasible, and worthwhile, but there's actually no bug to trigger currently because the misused values aren't used anywhere.

Expected behaviour
NEO correctly loads both current files with clock values corresponding to sample counts (increment 1 per sample, 1 second = 30_000) and upcoming files with clock values corresponding to nanoseconds (increment 33_333 per sample, 1 second = 1_000_000_000).

Additional context

Here are the errors I have identified.

Error 1:

python-neo/neo/rawio/blackrockrawio.py

Lines 1898 to 1900 in 7fc30cb

	'sampling_rate':
	self.__nsx_basic_header[nsx_nb]['timestamp_resolution']
	/ self.__nsx_basic_header[nsx_nb]['period'] * pq.Hz,

The 'sampling_rate' entry in the nsx_parameters is calculated as timestamp_resolution / period. This should be sample_resolution / period. However, as nsx files don't have sample resolution, this must be 30_000 / period.

Luckily, nsx_parameters['sampling_rate'] doesn't appear to be used anywhere! In fact, the location we might expect it to be used actually calculates sampling rate denovo using exactly 30_000 / period.

python-neo/neo/rawio/blackrockrawio.py

Lines 365 to 366 in 7fc30cb

	sr = float(main_sampling_rate / self.__nsx_basic_header[nsx_nb]['period'])
	self.sig_sampling_rates[nsx_nb] = sr

I propose in the header-based calculation of 'sampling_rate' that the 'timestamp_resolution' is replaced with hard-coded 30_000 and this calculated sample rate is used in the nsx load body via: self.__nsx_params[self.__nsx_spec[nsx_nb]]('sampling_rate', nsx_nb)

Error 2:
'time_unit' immediately following above

python-neo/neo/rawio/blackrockrawio.py

Lines 1901 to 1903 in 7fc30cb

	'time_unit': pq.CompoundUnit("1.0/{}*s".format(
	self.__nsx_basic_header[nsx_nb]['timestamp_resolution']
	/ self.__nsx_basic_header[nsx_nb]['period']))}

I suspect that this should be 1 / 30_000, based on how waveform_time_unit is used, but I don't actually know because it isn't used anywhere!

[zm711]

@cboulay,

I'm just reading through old issues. I haven't read this in detail, but did you 1) address this in your recent PR or 2) would be willing to address this in a future PR?

[zm711]

@cboulay,

just wanted to bump this one more time. Did you fix the issues you have pointed out in your recent PRs such that this can be closed? At minimum a small PR indicating with comments why you believe there is a problem would be nice.

[cboulay]

Hey, sorry for being absent on this. Yes, I'm fairly certain this is solved with #1338