-
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
Ensure empty stream segments are initialised #129
Open
jamieforth
wants to merge
11
commits into
xdf-modules:main
Choose a base branch
from
jamieforth:fix/expose-segments
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+405
−101
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1e59368
Ensure empty stream segments are initialised
jamieforth 4dd292a
Refactor file loading tests to use skip_if_file_not_found fixture
jamieforth 3a472ca
Refactor file loading tests to use a global dictionary of paths
jamieforth e0304d9
Update test file names: Remove `data_with_` prefix
jamieforth 84339da
More systematically test file data
jamieforth d4946d8
Fix effective sample rate when dejitter_timestamps=False
jamieforth 401ee45
Add skipif
cbrnr 387e813
Cosmetics
cbrnr 0141684
Change effective_srate when dejitter_timestamps=True
jamieforth 372d615
Update empty_streams test
jamieforth e5e2f13
Format comment
jamieforth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are you sure it wasn't correct before? Below, the
- 1
is needed because it stores the start and stop indices of segments, but here we're calculating the duration. If you only have 1 sample, you will now always get aneffective_srate
of0
(previously it was1 / duration
, which I think is the correct value).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.
If you only have 1 sample, the duration is 0, so wouldn't you end up with
1/0
in that case?In my view you can only calculate a duration with two or more samples, and
if len(stream.time_stamps) > 1
assures this.Without
-1
the effective sample rate inminimal
is calculated as 11.25 Hz but it should be 10 Hz, so I think the -1 is correct.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.
Yes, you are right!
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.
OK cool, I just push a change for the calculation of
effective_srate
whendejitter_timestamps=True
as well.This one was doing
len(time_stamps) / (duration + tdiff)
, which is fine when time-stamps are perfect, but off otherwise.Perfect clock
=>
1.0
# Correct=>
1.0
# CorrectFast clock
=>
1.08
# Incorrect=>
1.1
# CorrectSlow clock
=>
0.919
# Incorrect=>
0.9
# Correct