-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/expose segments #121
Fix/expose segments #121
Conversation
This ensures the length of empty streams is zero, and not equal to the number of channels.
* Ensure segment array is initialised only once * Empty streams return empty segment arrays
@agricolab do you have time to look? |
@@ -364,10 +364,6 @@ def load_xdf( | |||
) | |||
|
|||
# perform jitter removal if requested | |||
for tmp in temp.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏼
@@ -382,6 +378,10 @@ def load_xdf( | |||
stream.effective_srate = len(stream.time_stamps) / duration | |||
else: | |||
stream.effective_srate = 0.0 | |||
# initialize segment list in case jitter_removal was not selected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏼
catches empty streams and returns [] instead of [0,-1]
@@ -629,6 +629,7 @@ def _jitter_removal(streams, threshold_seconds=1, threshold_samples=500): | |||
for stream_id, stream in streams.items(): | |||
stream.effective_srate = 0 # will be recalculated if possible | |||
nsamples = len(stream.time_stamps) | |||
stream.segments = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏼 catches empty streams and returns [] instead of [0,-1] (when _jitter_removal
was called, i.e. dejitter_timestamps=True
@@ -348,7 +348,7 @@ def load_xdf( | |||
if stream.fmt == "string": | |||
stream.time_series = [] | |||
else: | |||
stream.time_series = np.zeros((stream.nchns, 0)) | |||
stream.time_series = np.zeros((0, stream.nchns)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏼 good catch. we probably need a test for empty streams eventually to catch such issues
Did a manual test and it passes now unduplicated segments
We might need a test with an empty stream though, eventually. |
Thanks @jamieforth ! |
Thanks @jamieforth and @agricolab! Does the test you mentioned pass with this change but fail without it? If so, could you add it to |
Yes, could be a good regression test. I'd also like if we'd had a minimal.xdf (or empty.xdf) with an empty stream. |
This addresses a couple of small issues with segment metadata.
When
dejitter_timestamps=False
reported segments now have the correct sample stop.When
dejitter_timestamps=True