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.
| 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?
| # 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() |
| 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.
| 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?
|
|
||
| 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.