Repository navigation
Select ann2rr intervals by time in the record - #622
Conversation
The start_time and stop_time arguments of ann2rr were compared with the length of each interval rather than with its position in the record, so start_time=0.9 dropped every interval shorter than 0.9 seconds. Select annotations between start_time and stop_time instead, as the WFDB ann2rr program's -f and -t options do, and measure the first interval from start_time. Fixes MIT-LCP#617.
tompollard
left a comment
There was a problem hiding this comment.
Thanks, this looks great! I compared it with the WFDB ann2rr program (-f and -t, with and without -A) on records 100, 1003, 201, 203 and 232, and the intervals match exactly for every normal time range.
Two optional suggestions:
-
With
start_timeafterstop_time, the result is silently empty (the C program swaps them). Raising aValueErrormight be clearer:if start_time is not None and stop_time is not None and start_time > stop_time: raise ValueError("start_time must not be after stop_time")
-
round()rounds halves to even, while the WFDB library rounds them up ((long)(x*f + 0.5)), so a time exactly half a sample in can give a different sample (for examplestart_time=0.002at 250 Hz gives 0 here and 1 inann2rr).int(start_time * ann.fs + 0.5)would match exactly. This is a very small edge case, so feel free to leave it.
(stop_time=0 gives an empty result here, while -t 0 means "no limit" in the C program. I think the behaviour here is the right one.)
Suggested in review: raise a ValueError instead of returning nothing when the range is reversed, and round half a sample up as the WFDB library does.
|
Thanks for checking against the C program! I took both suggestions:
I agree on |
|
PhysioNet is having a moment, which I assume is why tests are failing. Sorting it out now... |
tompollard
left a comment
There was a problem hiding this comment.
Thanks, this looks great! I re-ran the comparison with ann2rr and everything still matches.
Fixes #617.
ann2rr'sstart_timeandstop_timewere compared with the length of each interval, not its position in the record. They now select intervals by when they end in the record, matching the WFDBann2rrprogram's-fand-toptions:start_timetostop_time(both inclusive, as inann2rr) are usedstart_time(from 0 when it isn't given, as before)With record 100 (360 Hz):
start_time=0.9stop_time=0.5A parametrized test covers start only, stop only, both, a beat exactly at the start time (a zero-length first interval, as in
ann2rr) and a range with no beats. Four of the cases fail without the change.