Add gap_tolerance_ms API to Neuralynx reader#1822
Conversation
|
This is heroic! |
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
I think my biggest question is the overall design choice here. could you explain
- why handle legacy vs modern as two different if/else pathways deep in the code vs just taking the legacy values and encapsulating them in the modern version
- Why bounce back and further between ms and us? (mostly just to remind me). If you do switch back and further I think you'll need to make sure anything that reaches the user will show up as ms (not us)
|
|
||
| @staticmethod | ||
| def build_for_ncs_file(ncsMemMap, nlxHdr, gapTolerance=None, strict_gap_mode=True): | ||
| def build_for_ncs_file(ncsMemMap, nlxHdr, gap_tolerance_us=None, **kwargs): |
There was a problem hiding this comment.
above you start with milliseconds and now you switch to microseconds.
There was a problem hiding this comment.
The microseconds isn't a choice I'm making here, it's the factory's existing convention. NcsSectionsFactory has always worked in microseconds because NCS record timestamps are natively integer microseconds, and its gap math was already in us before this PR (the old parameter was just an unlabeled gapTolerance). I'm respecting that internal unit and only renamed it to gap_tolerance_us to make it explicit. The single ms->us conversion happens once at the reader boundary, so the user never leaves milliseconds; only the internal layer that was always in us stays in us. This is basically to respect the other contributor way of thinking inside the internals.
| if gapTolerance is not None: | ||
| warnings.warn( | ||
| "The `gapTolerance` parameter is deprecated and will be removed in version 0.16. " | ||
| "Use `gap_tolerance_us` instead.", |
There was a problem hiding this comment.
again how will this get exposed if the top-level function is in milliseconds?
There was a problem hiding this comment.
gap_tolerance_us is the factory-level parameter, not the user-facing one. End users go through NeuralynxRawIO(gap_tolerance_ms=...); this deprecation message is only reachable by code that calls the factory's old gapTolerance directly, which is internal. Fair point on consistency though: I can make the message point end users at the reader's gap_tolerance_ms so there's no ambiguity about which unit belongs where.
| # this is the old behavior, maybe we could put 0.9 sample interval no ? | ||
| gapTolerance = 0 | ||
| if gap_tolerance_us is None: | ||
| if strict_gap_mode is not None and not strict_gap_mode: |
There was a problem hiding this comment.
Shouldn't we instead of using the deprecated variable name in the code we should just convert the deprecated code into the current code at the top somewhere with like two lines of code. Then we only check on the new code lines so when we are done deprecating it we delete the couple lines at the top rather than a big rewrite here.
| def _format_gap_report(self, detected_gaps, sampling_frequency, filename): | ||
| """ | ||
| Format a detailed gap report showing where timestamp discontinuities occur. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| detected_gaps : list of (int, int) | ||
| List of (record_index, gap_size_us) tuples from NcsSections.detected_gaps. | ||
| sampling_frequency : float | ||
| Sampling frequency in Hz used for the file. | ||
| filename : str | ||
| Path to the NCS file for the report header. | ||
|
|
||
| Returns | ||
| ------- | ||
| str | ||
| Formatted gap report with table. | ||
| """ | ||
| gap_durations_ms = [abs(gap_size) / 1000.0 for _, gap_size in detected_gaps] | ||
| gap_positions_seconds = [ | ||
| record_index * NcsSection._RECORD_SIZE / sampling_frequency | ||
| for record_index, _ in detected_gaps | ||
| ] | ||
|
|
||
| gap_detail_lines = [ | ||
| f"| {record_index:>15,} | {pos:>21.6f} | {dur:>21.3f} |\n" | ||
| for (record_index, _), pos, dur in zip(detected_gaps, gap_positions_seconds, gap_durations_ms) | ||
| ] | ||
|
|
||
| return ( | ||
| f"Gap Report for {os.path.basename(filename)}:\n" | ||
| f"Found {len(detected_gaps)} timestamp gaps " | ||
| f"(detection threshold: {NcsSectionsFactory._maxGapSampFrac} sample intervals)\n\n" | ||
| "Gap Details:\n" | ||
| "+-----------------+-----------------------+-----------------------+\n" | ||
| "| Record Index | Record at (Seconds) | Gap Size (ms) |\n" | ||
| "+-----------------+-----------------------+-----------------------+\n" | ||
| + "".join(gap_detail_lines) | ||
| + "+-----------------+-----------------------+-----------------------+\n" | ||
| ) |
There was a problem hiding this comment.
I have a hard time reading this, but it looked fine in Intan so I trust you for this bit of code.
| chan_uid = (channel["name"], channel["id"]) | ||
|
|
||
| data = self._sigs_memmaps[seg_index][chan_uid] | ||
| return data["timestamp"].copy() |
There was a problem hiding this comment.
Cheap. It copies only the timestamp column: one uint64 per 512-sample record, so roughly 1/512 of the data size (tens of KB for a typical file, not the sample array). The .copy() is deliberate so the returned array owns its memory and isn't a live view into the file memmap.
| return None, None, None | ||
|
|
||
| # Determine gap tolerance in microseconds for NcsSectionsFactory | ||
| if self._use_legacy_gap_mode: |
There was a problem hiding this comment.
Again I'm not sure if I like this strategy overall is one I like. Rather than decide between legacy vs modern why not just make "modern" handle the legacy case so that we can slowly delete all legacy based code. You have this as a private variable which is better than making it public, but I think the strategy I would prefer would be:
def (a, b, c)
if c is not None:
b= c if c is x else y
This way right at the top we handle the legacy case by converting the b variable based on the deprecated c. Then when the deprecation window is over we just delete c and the top little bit of code.
There was a problem hiding this comment.
I consolidated the value translation into the factory (see above), but the reader-level _use_legacy_gap_mode flag can't be collapsed the same way, because it isn't only routing: it also guards a genuine behavior difference. In legacy mode (a user passed strict_gap_mode) the reader deliberately skips the new error-on-gaps default so deprecated code keeps its old behavior; in modern mode it applies error-by-default. Collapsing the flag would silently change what deprecated calls do. The reader also can't pre-translate strict_gap_mode into a numeric tolerance up front, because the value depends on per-file acquisition type and frequency that only the factory sees. So the reader forwards the deprecated parameter to the factory, where it's now handled in one place, and keeps the flag only to preserve legacy behavior.
| factory_kwargs = {"strict_gap_mode": self.strict_gap_mode} | ||
| elif self.gap_tolerance_ms is not None: | ||
| # New API: convert ms to us | ||
| factory_kwargs = {"gap_tolerance_us": self.gap_tolerance_ms * 1000.0} |
There was a problem hiding this comment.
This is what I'm trying to understand. Won't it be confusing for the user if they are requesting a gap in ms but then internal we do everything on us? If we give them an error in us they might be confused no?
There was a problem hiding this comment.
The conversion here is ms -> us only to feed the factory. The user never sees us: the gap report is explicitly emitted in ms (gap size) and seconds (position in the recording), so an error is already in the unit they passed. I'll double-check every user-facing string stays in ms.
|
|
||
| entities_to_test = [] # list of files to test compliance | ||
| entities_to_download = [] # when files are at gin | ||
| io_kwargs = {} # extra kwargs passed to ioclass constructor |
There was a problem hiding this comment.
Yeah this seems good. If we are trying to add gap tolerance everywhere then it makes sense to allow us to slowly out io_kwargs. Or to play with the tolerance as needed. This looks good.
4 participants
Adds the
gap_tolerance_msparameter toNeuralynxRawIOandNeuralynxIO, following the pattern established by the Blackrock reader in #1789. When gaps are detected and no tolerance is set, aValueErroris raised with a detailed gap report. When a tolerance is provided, the data is automatically segmented at gaps exceeding that threshold. The oldstrict_gap_modeparameter is deprecated with a warning. By default (gap_tolerance_ms=None), only gaps of at least one sample period are reported as errors; sub-sample timestamp deviations from rounding or clock jitter are silently ignored.A
_get_neuralynx_timestamps()method exposes the original per-record hardware timestamps in microseconds, giving users access to the raw timing information for drift analysis or precise alignment with external systems.The test infrastructure gains
rawio_kwargsonBaseTestRawIOandio_kwargsonBaseTestIO, allowing any format's tests to pass constructor arguments without overriding test methods or subclassing the IO class.