Update MovingWindow to add resampler #272
Merged
Conversation
This file contains hidden or 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
github-actions
bot
added
part:data-pipeline
Contributor
Author
|
This is a draft which only updates the size parameter type to timedelta of the MovingWindow at the moment. |
daniel-zullo-frequenz
requested review from
matthias-wende-frequenz
and removed request for
sahas-subramanian-frequenz
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
8 times, most recently
from
cca7aa2 to
8ac2f18
Compare
Contributor
matthias-wende-frequenz
left a comment
matthias-wende-frequenz
left a comment
There was a problem hiding this comment.
This is looking quite good already. I'll have another look once the commit title doesn't say dirty any more :).
Contributor
Author
Cool, good to know that! Then it will 'beautify' the current changes and add units tests. I'll let you know once it's ready to review |
shsms
reviewed
Mar 27, 2023
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
3 times, most recently
from
6cdf005 to
13cf9a2
Compare
daniel-zullo-frequenz
requested review from
leandro-lucarella-frequenz,
mathias-baumann-frequenz,
matthias-wende-frequenz and
shsms
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
from
13cf9a2 to
96aa37f
Compare
leandro-lucarella-frequenz
previously approved these changes
Apr 5, 2023
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
from
d872415 to
32f4f52
Compare
Contributor
Author
|
Rebased onto latest v0.x.x branch |
leandro-lucarella-frequenz
previously approved these changes
Apr 6, 2023
matthias-wende-frequenz
previously approved these changes
Apr 6, 2023
Contributor
matthias-wende-frequenz
left a comment
matthias-wende-frequenz
left a comment
There was a problem hiding this comment.
just one last comment :)
| documentation. | ||
| Resampling might be required to reduce the number of samples to store | ||
| without losing precision, and it can be set by specifying the resampler |
Contributor
There was a problem hiding this comment.
I'm not sure what you want to say without loosing precision. We are loosing precision for when we downsample but that's acceptable in many cases.
Contributor
Author
There was a problem hiding this comment.
I meant that we can reduce the number of samples without losing much precision through resampling. I'd then remove without losing precision as it is confusing and it isn't generic in the sense it only applies for downsampling where it is somehow implicit an acceptable loss of precision
daniel-zullo-frequenz
dismissed stale reviews from matthias-wende-frequenz and leandro-lucarella-frequenz
via
1cf572d
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
2 times, most recently
from
1cf572d to
5e073fe
Compare
Contributor
Author
|
Updated considering the latest comment from Matthias |
Signed-off-by: Daniel Zullo <[email protected]>
Modify the MovingWindow class to accept the size parameter as a timedelta object instead of an integer. This change allows the size parameter to represent the time span of the moving window over which samples will be stored, making it more intuitive for users and consistent among other parameters in the MovingWindow class. Signed-off-by: Daniel Zullo <[email protected]>
There is not need at the moment to keep the state of the flag to prevent deep copying of the adjacent buffer in MovingWindow. Signed-off-by: Daniel Zullo <[email protected]>
Signed-off-by: Daniel Zullo <[email protected]>
Signed-off-by: Daniel Zullo <[email protected]>
Add a resampler in the MovingWindow to control the granularity of the samples to be stored in the underlying buffer. Signed-off-by: Daniel Zullo <[email protected]>
There were inconsistencies in the way the SDK set datetime variables in the MovingWindow/RingBuffer without tz/tzinfo and in the Resampler with tz/tzinfo. This patch sets the timezone UTC for all datetime variables set in the MovingWindow and RingBuffer to make them consistent to the datetime variables set in the Resampler. Signed-off-by: Daniel Zullo <[email protected]>
Signed-off-by: Daniel Zullo <[email protected]>
Signed-off-by: Daniel Zullo <[email protected]>
Change the way the markers are represented in RingBuffer so that `replace()` is not called every time the markers are assigned or compared to. Signed-off-by: Daniel Zullo <[email protected]>
The ISO format is more precise and might be less confusing for the users. Signed-off-by: Daniel Zullo <[email protected]>
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
from
5e073fe to
3bc00be
Compare
The test fails when it is run in the CI, increasing the sampling won't solve the problem, just it will fail less ofter. There are already two different open issues to address and fix these flaky tests. See the links below: frequenz-floss#249 frequenz-floss#70 Signed-off-by: Daniel Zullo <[email protected]>
daniel-zullo-frequenz
force-pushed
the
feature/add-resampler-moving-window
branch
from
f6ef466 to
5d37033
Compare
leandro-lucarella-frequenz
approved these changes
Apr 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Fixes #269