Fixes on copy behavior in ring buffer window method #638

cwasicki · 2023-09-04T16:55:22Z

This PR introduces several copy-related updates to the ring buffer window functionality

Changes:

Default Copy Behavior: Changed the default behavior to copy data in the ring buffer window. This is to prioritize data integrity over performance, which is a minor concern for most expected use-cases. Made force_copy as a keyword argument.
Moved the logic for extracting wrapped buffers into a separate method to fix a bug and allow for better testing and future maintainability.
Handling None Values: Fixed an issue where None values were being forcibly copied in the ring buffer window. The decision to enforce a copy is now left to the user, even when the data contains None values.
Test Coverage: Added tests for the window method in the ring buffer, including tests for expected copy behavior on missing values.
Fix to return an empty array in case start = end in ring buffer window.
Expose restricted ring buffer window by moving window.

Part of #214

cwasicki · 2023-09-08T16:50:44Z

Rebased @matthias-wende-frequenz

cwasicki · 2023-09-11T21:01:42Z

@matthias-wende-frequenz Ready for review

matthias-wende-frequenz

Just two small comments. Looks good otherwise!
Also needs a rebase.

matthias-wende-frequenz · 2023-09-12T14:18:57Z

src/frequenz/sdk/timeseries/_ringbuffer/buffer.py

+        )
+
+    @staticmethod
+    def _wrapped_buffer_window(


Does this have to be a static method? I'd suggest to make it a class method as long it's not used outside the class context.

Why do you think so, or what is the advantage of a classmethod? I made it static since it's an isolated algorithmic piece which does not need any internal state of the class. Only the buffer argument would change here.

The main motivation was testing purpose (actually contained multiple bugs before), i.e. separate wrapping algorithm from index logic (which will become more tricky in follow-ups). But it can also easily be stripped out when needed elsewhere.

One example for outside usage I could imagine is the periodic feature extractor, which I assume is using similar code.

You are right. It doesn't depend on the internal state. Let's keep it as is.

matthias-wende-frequenz · 2023-09-12T14:41:38Z

tests/timeseries/test_moving_window.py

+    return datetime.fromtimestamp(i, tz=timezone.utc)
+
+
 async def test_access_window_by_index() -> None:


I wonder if that's the correct function to add the tests. This was supposed to access the window by integer index, but the tests are accessing by datetime object.

The newly added datetime slice tests were added in test_access_window_by_ts_slice. Then a single failure test to for int slices in test_access_window_by_int_slice

Sorry I missed that. All good then ;).

Test for expected copy behavior on missing values does not work as documented. Signed-off-by: cwasicki <[email protected]>

The functionality was not working as intended. Moreover the decision whether to enforce a copy or not should be left to the user even if the data contains None values. Signed-off-by: cwasicki <[email protected]>

This just moves the logic into a separate function to allow better testing of the logic. Bug found in the logic will be fixed in later commit. Signed-off-by: cwasicki <[email protected]>

This reworks the method for extracting wrapped buffer to fix a bug in the existing copy behavior for a specific case. Signed-off-by: cwasicki <[email protected]>

Changing the default is motivated by favouring data integrity over performance concerns which are minor in the majority of the expected applications. Also make force_copy a keyword argument. Signed-off-by: cwasicki <[email protected]>

Restore the typical list-like behavior for equal start and end. Would be inconsistent with the forbidden case start > end. Signed-off-by: cwasicki <[email protected]>

Expose window functionality of underlying buffer. Only datetime indices are supported so far. Signed-off-by: cwasicki <[email protected]>

Signed-off-by: cwasicki <[email protected]>

cwasicki requested a review from a team as a code owner September 4, 2023 16:55

cwasicki requested a review from daniel-zullo-frequenz September 4, 2023 16:55

github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Sep 4, 2023

cwasicki force-pushed the rbuffer-copy branch 2 times, most recently from 74ba702 to 1f914df Compare September 4, 2023 19:08

github-actions bot added the part:docs Affects the documentation label Sep 4, 2023

cwasicki added this to the v1.0.0-rc milestone Sep 4, 2023

cwasicki self-assigned this Sep 4, 2023

cwasicki requested a review from matthias-wende-frequenz September 4, 2023 19:28

cwasicki force-pushed the rbuffer-copy branch 2 times, most recently from 00e0e7e to 0bc072f Compare September 8, 2023 16:27

cwasicki force-pushed the rbuffer-copy branch 2 times, most recently from 8e26c95 to 48ccae1 Compare September 11, 2023 20:48

matthias-wende-frequenz reviewed Sep 12, 2023

View reviewed changes

cwasicki added 8 commits September 12, 2023 21:22

Add tests for window method in ring buffer

0ef746f

Test for expected copy behavior on missing values does not work as documented. Signed-off-by: cwasicki <[email protected]>

Do not force copy on Nones in ring buffer window

54101c8

The functionality was not working as intended. Moreover the decision whether to enforce a copy or not should be left to the user even if the data contains None values. Signed-off-by: cwasicki <[email protected]>

Add separate method for extracting wrapped buffers

a4a59c4

This just moves the logic into a separate function to allow better testing of the logic. Bug found in the logic will be fixed in later commit. Signed-off-by: cwasicki <[email protected]>

Fix copy behavior when extracting wrapped buffers

c584d25

This reworks the method for extracting wrapped buffer to fix a bug in the existing copy behavior for a specific case. Signed-off-by: cwasicki <[email protected]>

Copy by default in ring buffer window

4d62bca

Changing the default is motivated by favouring data integrity over performance concerns which are minor in the majority of the expected applications. Also make force_copy a keyword argument. Signed-off-by: cwasicki <[email protected]>

Fix start equal end in ring buffer window

61ddef1

Restore the typical list-like behavior for equal start and end. Would be inconsistent with the forbidden case start > end. Signed-off-by: cwasicki <[email protected]>

Add window function to moving window

1970178

Expose window functionality of underlying buffer. Only datetime indices are supported so far. Signed-off-by: cwasicki <[email protected]>

Update release notes

4d8de2a

Signed-off-by: cwasicki <[email protected]>

cwasicki force-pushed the rbuffer-copy branch from 48ccae1 to 4d8de2a Compare September 12, 2023 19:22

matthias-wende-frequenz approved these changes Sep 13, 2023

View reviewed changes

matthias-wende-frequenz added this pull request to the merge queue Sep 13, 2023

Merged via the queue into frequenz-floss:v0.x.x with commit bf1b22c Sep 13, 2023

cwasicki deleted the rbuffer-copy branch September 13, 2023 10:32

cwasicki mentioned this pull request Sep 13, 2023

MovingWindow: Create Interface for requesting data and handling the data copy behaviour #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes on copy behavior in ring buffer window method #638

Fixes on copy behavior in ring buffer window method #638

Uh oh!

cwasicki commented Sep 4, 2023 •

edited

Loading

Uh oh!

cwasicki commented Sep 8, 2023

Uh oh!

cwasicki commented Sep 11, 2023

Uh oh!

matthias-wende-frequenz left a comment •

edited

Loading

Uh oh!

matthias-wende-frequenz Sep 12, 2023

Uh oh!

cwasicki Sep 12, 2023

Uh oh!

cwasicki Sep 12, 2023

Uh oh!

matthias-wende-frequenz Sep 13, 2023

Uh oh!

matthias-wende-frequenz Sep 12, 2023

Uh oh!

cwasicki Sep 12, 2023

Uh oh!

matthias-wende-frequenz Sep 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		return datetime.fromtimestamp(i, tz=timezone.utc)


		async def test_access_window_by_index() -> None:

Fixes on copy behavior in ring buffer window method #638

Fixes on copy behavior in ring buffer window method #638

Uh oh!

Conversation

cwasicki commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwasicki commented Sep 8, 2023

Uh oh!

cwasicki commented Sep 11, 2023

Uh oh!

matthias-wende-frequenz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthias-wende-frequenz Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

cwasicki Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

cwasicki Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

matthias-wende-frequenz Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

matthias-wende-frequenz Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

cwasicki Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

matthias-wende-frequenz Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cwasicki commented Sep 4, 2023 •

edited

Loading

matthias-wende-frequenz left a comment •

edited

Loading