Simple ringbuffer with tests #116

mathias-baumann-frequenz · 2022-12-13T17:05:25Z

First draft, more tests to come, also performance test.
next step is testing the slicing parts.

Signed-off-by: Marenz mathias.baumann@frequenz.com

src/frequenz/sdk/util/ringbuffer.py

ela-kotulska-frequenz · 2022-12-14T10:04:07Z

src/frequenz/sdk/util/ringbuffer.py

+    def write(self, value: T, index: int) -> None:
+        """"""
+        self.data[index] = value


You can also override __setitem__ :)

Maybe check if index < len(self.data)?

src/frequenz/sdk/util/ringbuffer.py

mathias-baumann-frequenz · 2022-12-16T17:20:35Z

I pushed an update that shows a basic working of the timestamp buffer. The numpy variant still has a problem that I am currently looking into.

More logic tests are to come as well as performance tests

mathias-baumann-frequenz · 2022-12-20T14:33:54Z

Array: ........................................
List:  ........................................
Time to fill 29 days with data:
Array: 0.09143387260000964 seconds
List:  0.08543843417501193 seconds
Diff:  0.005995438424997709

       =========================================
Array: ........................................
List:  ........................................
Filling 29 days and running average & mean on every day:
Array: 0.09528444537500036 seconds
List:  0.12714749602500888 seconds
Diff:  -0.03186305065000852

leandro-lucarella-frequenz

A lot of the comments are just me putting the pylint or mypy hat, so it is not as bad as it looks 😆

leandro-lucarella-frequenz · 2022-12-21T13:18:43Z

benchmarks/benchmark_ringbuffer.py

+# Copyright © 2022 Frequenz Energy-as-a-Service GmbH
+
+"""
+Performance test for the `Ringbuffer` class


Please mirror the directory structure in src for benchmarks so it is easier to figure out which benchmark is benchmarking what. In this case it should be benchmarks/util/ringbuffer.py

I didn't see power_distribution in the src dir so I assumed it would be more.. arbitrary..

I think at some point that one was top-level, we probably forgot to update the benchmarks structure when we moved it.

Not sure if it is intentional but the benchmark disappeared from the PR.

Yeah no, that was a bad renaming

benchmarks/benchmark_ringbuffer.py

leandro-lucarella-frequenz · 2022-12-21T13:22:23Z

benchmarks/benchmark_ringbuffer.py

+            (f"{basetime + timedelta(days=day)} average {np.average(minutes)}")
+            (f"{basetime + timedelta(days=day)} median {np.median(minutes)}")


What are these strings laying around there?

I was just trying to prevent python from optimizing them away

I don't get it? You want to test the performance of building a formatted string?

benchmarks/benchmark_ringbuffer.py

leandro-lucarella-frequenz · 2022-12-21T13:26:49Z

src/frequenz/sdk/util/ringbuffer.py

+
+
+class RingBuffer(Generic[T]):
+    def __init__(self, container: Any):


You probably want to have some restrictions on the container, like MutableSequence?

I'd love to, but I found no type that would fit so far. Will investigate again.

tests/utils/ringbuffer.py:19: error: Argument 1 to "RingBuffer" has incompatible type "ndarray[Any, dtype[f loating[_64Bit]]]"; expected "MutableSequence[<nothing>]" [arg-type]

Can you show how you changed the code to get that message?

I just changed Any to MutableBuffer[T]

OK, yeah, it looks like it's not even a Sequence (although it should), but probably won't get all the methods needed by a MutableSequence: numpy/numpy#2776.

So we have 2 options I can see:

Use an union: MutableSquence[T] | np.ndarray, which is a bit ugly to have this special casing, why only np.ndarray and not anything else that looks like a duck?

Use a Protocol: just look at all the methods and attributes you need a container to have an write a BufferContainer protocol.

IMHO 2. is the correct option, but it is a bit of work that might not be justified at this point, so I would leave Any and create an issue to improve this in the future.

src/frequenz/sdk/util/ringbuffer.py

leandro-lucarella-frequenz · 2022-12-21T13:57:19Z

src/frequenz/sdk/util/ringbuffer.py

+        """
+        return index % self.max_size()
+
+    def __getitem__(self, index_or_slice: int | slice) -> T | Sequence[T]:


Same as before, better not provide this if the implementation is not correct IMHO.

Well, it is correct insofar as that it provides direct access to the buffer..

Same comment as above, I think it is more clear to provide access to the underlying container and let people slice that directly instead.

Another comment is you can improve type checking by adding @overloads, to specify which subset of types are valid in those unions. Here is an example: https://github.com/frequenz-floss/frequenz-sdk-python/pull/130/files#diff-43d601005020a1afe419acdfad091a0e813ddd9a6f2e1d4e0db73c030ff8d599R135-R143

tests/utils/ringbuffer.py

leandro-lucarella-frequenz · 2022-12-21T14:05:07Z

Beware of damn GitHub compressing comments and hiding them, make sure to click when it says "N hidden conversations. Load more".

leandro-lucarella-frequenz

There were a few pending unanswered comments from the previous review. I think all the remaining stuff is very minor. There are only 2 major topics:

If, when and how do we need the sorted buffer.
If we really need proper slicing (setting and getting). My impression is we don't need setting a slice. About getting, not sure, maybe, maybe it is enough with using islice and we save some complexity (which IMHO while implementing the resampling buffer was not at all minor, specially with the many cases if we include steps and negative values).

src/frequenz/sdk/util/ringbuffer.py

Signed-off-by: Marenz <mathias.baumann@frequenz.com>

mathias-baumann-frequenz · 2022-12-27T14:18:39Z

Closing this to keep the branch around for the simple ring buffer, will create a new PR with only the sorted ring buffer

ela-kotulska-frequenz reviewed Dec 14, 2022

View reviewed changes

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch from 16ad023 to 67541ec Compare December 14, 2022 17:34

github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Dec 14, 2022

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch from 67541ec to 35544a8 Compare December 16, 2022 17:19

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch 2 times, most recently from bc0a8ee to f3d0b72 Compare December 20, 2022 14:12

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch 2 times, most recently from 96de0fa to 2057ab9 Compare December 20, 2022 16:30

leandro-lucarella-frequenz reviewed Dec 21, 2022

View reviewed changes

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch from 2057ab9 to 64c2a6b Compare December 21, 2022 16:43

leandro-lucarella-frequenz added the part:core Affects the SDK core components (data structures, etc.) label Dec 21, 2022

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch from 64c2a6b to 123807e Compare December 22, 2022 16:46

github-actions bot removed the part:core Affects the SDK core components (data structures, etc.) label Dec 22, 2022

matthias-wende-frequenz mentioned this pull request Dec 23, 2022

Handle data from disk #132

Closed

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch 2 times, most recently from 0d16f7f to ffa7b7c Compare December 23, 2022 13:08

leandro-lucarella-frequenz reviewed Dec 23, 2022

View reviewed changes

src/frequenz/sdk/util/ringbuffer.py Outdated Show resolved Hide resolved

Simple ringbuffer with tests

80bbd4d

Signed-off-by: Marenz <mathias.baumann@frequenz.com>

mathias-baumann-frequenz force-pushed the simple-ringbuffer branch from 0464117 to 80bbd4d Compare December 27, 2022 14:11

mathias-baumann-frequenz closed this Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple ringbuffer with tests #116

Simple ringbuffer with tests #116

mathias-baumann-frequenz commented Dec 13, 2022

ela-kotulska-frequenz Dec 14, 2022

mathias-baumann-frequenz commented Dec 16, 2022

mathias-baumann-frequenz commented Dec 20, 2022

leandro-lucarella-frequenz left a comment

leandro-lucarella-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 23, 2022

mathias-baumann-frequenz Dec 23, 2022

leandro-lucarella-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 22, 2022

leandro-lucarella-frequenz Dec 21, 2022

mathias-baumann-frequenz Dec 21, 2022

leandro-lucarella-frequenz Dec 21, 2022

leandro-lucarella-frequenz commented Dec 21, 2022 •

edited

leandro-lucarella-frequenz left a comment

mathias-baumann-frequenz commented Dec 27, 2022

		(f"{basetime + timedelta(days=day)} average {np.average(minutes)}")
		(f"{basetime + timedelta(days=day)} median {np.median(minutes)}")



		class RingBuffer(Generic[T]):
		def __init__(self, container: Any):

Simple ringbuffer with tests #116

Simple ringbuffer with tests #116

Conversation

mathias-baumann-frequenz commented Dec 13, 2022

Choose a reason for hiding this comment

mathias-baumann-frequenz commented Dec 16, 2022

mathias-baumann-frequenz commented Dec 20, 2022

leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandro-lucarella-frequenz commented Dec 21, 2022 • edited

leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

mathias-baumann-frequenz commented Dec 27, 2022

leandro-lucarella-frequenz commented Dec 21, 2022 •

edited