Skip to content
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

Add capacity, oldest and newest timestamp to moving window #598

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

cwasicki
Copy link
Collaborator

@cwasicki cwasicki commented Aug 23, 2023

The capacity of the moving window is exposed, which is the maximum number of values that the moving window can hold.

Also the oldest and newest timestamps are exposed by the moving window.

@cwasicki cwasicki requested a review from a team as a code owner August 23, 2023 13:40
@cwasicki cwasicki requested a review from llucax August 23, 2023 13:40
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Aug 23, 2023
@cwasicki cwasicki added this to the v1.0.0-rc milestone Aug 23, 2023
@github-actions github-actions bot added the part:docs Affects the documentation label Aug 23, 2023
@cwasicki cwasicki force-pushed the capacity branch 2 times, most recently from 7e367e2 to 8103652 Compare August 23, 2023 18:42
@cwasicki cwasicki changed the title Add capacity property to moving window Add capacity, oldest and timestamp to moving window Aug 23, 2023
@cwasicki cwasicki changed the title Add capacity, oldest and timestamp to moving window Add capacity, oldest and newest timestamp to moving window Aug 23, 2023
Copy link
Contributor

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I suggest to rename the newest and oldest timestamp functions in the ringbuffer too. Furthermore can you also please update the code in the MovingWindow that uses this functions to use the new ones?

@cwasicki cwasicki force-pushed the capacity branch 2 times, most recently from 39bfa84 to 92b8f20 Compare August 24, 2023 08:44
@cwasicki
Copy link
Collaborator Author

@matthias-wende-frequenz Updated.

@cwasicki cwasicki self-assigned this Aug 24, 2023
@llucax
Copy link
Contributor

llucax commented Aug 24, 2023

Removing myself as a reviewer, as @matthias-wende-frequenz already picked up on it.

@llucax llucax removed their request for review August 24, 2023 12:51
@@ -313,9 +352,9 @@ def __getitem__(self, key: SupportsIndex | datetime | slice) -> float | ArrayLik
key = slice(slice(key.start, key.stop).indices(self.__len__()))
elif isinstance(key.start, datetime) or isinstance(key.stop, datetime):
if key.start is None:
key = slice(self._buffer.time_bound_oldest, key.stop)
key = slice(self._buffer.oldest_timestamp, key.stop)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the ._buffer now ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't want to touch it since I am reworking this functionality at the moment for #214 where I remove this part. Also they are not exactly the same as they differ for empty arrays (which is not a concern here due to the check couple of lines above).

@@ -173,11 +173,11 @@ def _timestamp_to_rel_index(self, timestamp: datetime) -> int:

# distance between the input ts and the ts of oldest known samples (in samples)
dist_to_oldest = int(
(timestamp - self._buffer.time_bound_oldest) / self._sampling_period
(timestamp - self._buffer.oldest_timestamp) / self._sampling_period
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. no need to access the property through the _buffer

Copy link
Contributor

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more small comments. Otherwise it's good to go!

@cwasicki cwasicki changed the title Add capacity, oldest and newest timestamp to moving window Add capacity to moving window Sep 4, 2023
@cwasicki
Copy link
Collaborator Author

cwasicki commented Sep 4, 2023

@matthias-wende-frequenz Removed the oldest timestamp method.

@cwasicki cwasicki changed the title Add capacity to moving window Add capacity and newest timestamp to moving window Sep 4, 2023
@cwasicki cwasicki added the status:blocked Other issues must be resolved before this can be worked on label Sep 6, 2023
@cwasicki cwasicki changed the title Add capacity and newest timestamp to moving window Add capacity, oldest and newest timestamp to moving window Sep 6, 2023
@cwasicki
Copy link
Collaborator Author

cwasicki commented Sep 6, 2023

Reworked based on #641.

Blocking until the above is merged.

The capacity is the maximum number of values that the moving window can
hold.

Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
@cwasicki cwasicki removed the status:blocked Other issues must be resolved before this can be worked on label Sep 6, 2023
@cwasicki
Copy link
Collaborator Author

cwasicki commented Sep 6, 2023

Rebased

@matthias-wende-frequenz matthias-wende-frequenz added this pull request to the merge queue Sep 8, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 9e8b167 Sep 8, 2023
@cwasicki cwasicki deleted the capacity branch September 8, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
Development

Successfully merging this pull request may close these issues.

3 participants