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.

Sign up for GitHub

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

Jump to bottom

Fix single element access for moving window #672

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Fix single element access for moving window #672

merged 3 commits into from
Sep 27, 2023

Conversation

cwasicki
Copy link
Collaborator

@cwasicki cwasicki commented Sep 14, 2023 •
edited
Loading

This brings full support for single element access in moving windows either via integer or datetime indices including bug fixes.

@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Sep 14, 2023
@cwasicki cwasicki self-assigned this Sep 14, 2023
@cwasicki cwasicki added this to the v1.0.0-rc2 milestone Sep 14, 2023
This was referenced Sep 14, 2023
Closed
Closed
@github-actions github-actions bot added part:docs Affects the documentation part:data-pipeline Affects the data pipeline labels Sep 21, 2023
@cwasicki cwasicki changed the title WIP: Fix int index access for moving window Fix single element access for moving window Sep 21, 2023
@cwasicki cwasicki marked this pull request as ready for review September 21, 2023 16:02
@cwasicki cwasicki requested a review from a team as a code owner September 21, 2023 16:02
@cwasicki cwasicki requested a review from Marenz September 21, 2023 16:02
@cwasicki cwasicki added the status:blocked Other issues must be resolved before this can be worked on label Sep 21, 2023
@cwasicki
Copy link
Collaborator Author

Blocking since based on #668.

@cwasicki cwasicki removed the status:blocked Other issues must be resolved before this can be worked on label Sep 27, 2023
@cwasicki
Copy link
Collaborator Author

Ready for review.

llucax
llucax reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

So at() is basically __getitem__() limited to one element (so no slices)? Any reason to keep it as a public method? For me it adds a bit of confusion, the same I'm wondering what's the difference with __getitem__(), other users could have the same doubt.

src/frequenz/sdk/timeseries/_moving_window.py
@@ -241,6 +241,43 @@ def capacity(self) -> int:
"""
return self._buffer.maxlen

def at(self, key: int | datetime) -> float: # pylint: disable=invalid-name
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually hate this check failing because names are short. Fortunately is going away in pylint 3.

image

https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/invalid-name.html#predefined-naming-patterns

We could disable it now by customizing the name regexes, but I'm not sure it is worth it.

@cwasicki
Copy link
Collaborator Author

cwasicki commented Sep 27, 2023 •
edited
Loading

So at() is basically getitem() limited to one element (so no slices)

Yes, there is at for single elements and window for slices. __getitem__ supports both and calls one or the other depending on the indices.

Any reason to keep it as a public method?

The reason I made it public is that it could also support extra arguments such as filling missing values like for the window method (stub here: #669).

@llucax
Copy link
Contributor

llucax commented Sep 27, 2023

OK, that makes sense, but then I would add a note in both cross-linking and quickly mentioning the differences.

@cwasicki
Copy link
Collaborator Author

Updated @llucax

llucax
llucax previously approved these changes Sep 27, 2023
View reviewed changes
src/frequenz/sdk/timeseries/_moving_window.py Outdated
"""
Return the sample at the given index or timestamp.

In contrast to the `window` method, which expects a slice as argument,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional, as I will still go through the code docs and improve them, but mentioning this so people get used to use cross references.

Suggested change
In contrast to the `window` method, which expects a slice as argument,
In contrast to the [`window`][frequenz.sdk.timeseries.MovingWindow.window] method, which expects a slice as argument,

@cwasicki
Enhance index access tests for moving window
a064c03 
More tests for single element access of the moving window are added including
tests that indicate bugs which should be fixed in follow-up commits.

Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
@cwasicki
Add at method for single element access of moving window
ed39f82 
This method implements corrected access of single elements in the moving
window. Similar to its `window` counterpart for slices, in future it
could provide additional features such as replacing missing values.

Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
@cwasicki
Use at method in getitem magic of moving window
7e87989 
The incomplete implementation of single element access in the getitem
magic is replaced by using the `at` method.

Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
@cwasicki
Copy link
Collaborator Author

Updated @llucax

@cwasicki cwasicki added this pull request to the merge queue Sep 27, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 5c98c35 Sep 27, 2023
@cwasicki cwasicki deleted the fix-elem branch September 27, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llucax llucax llucax approved these changes

@matthias-wende-frequenz matthias-wende-frequenz matthias-wende-frequenz left review comments

@Marenz Marenz Awaiting requested review from Marenz Marenz was automatically assigned from frequenz-floss/python-sdk-team

Assignees

@cwasicki cwasicki

Labels
part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
Python SDK Roadmap
Status: Done
Milestone
v1.0.0-rc2
Development

Successfully merging this pull request may close these issues.
3 participants
@cwasicki @llucax @matthias-wende-frequenz