frequenz-floss / frequenz-sdk-python

Frequenz Python Software Development Kit (SDK)
https://frequenz-floss.github.io/frequenz-sdk-python/
MIT License
13 stars 17 forks source link

Inconsistent gap treatment in ring buffer #646

Closed cwasicki closed 1 month ago

cwasicki commented 1 year ago

What happened?

If there is no data in the ring buffer (e.g. on start up) or no valid data (e.g. only None values), there is inconsistent behavior for gaps and __len__, which relies on gaps.

What did you expect instead?

Example on startup (expected 1 gap over the full window): https://github.com/frequenz-floss/frequenz-sdk-python/blob/01b8d9d1886f298106a505bf68f6c03ced380580/tests/timeseries/test_ringbuffer.py#L211

Example for no valid data (expected length of zero): https://github.com/frequenz-floss/frequenz-sdk-python/blob/01b8d9d1886f298106a505bf68f6c03ced380580/tests/timeseries/test_ringbuffer.py#L252

Affected version(s)

v0.x.x

Affected part(s)

Data pipeline (part:data-pipeline)

Extra information

No response

cwasicki commented 1 year ago

Adding it to rc2 milestone because it would ideally be addressed for v1.0 release. But could also be addressed later since it's a bug fix which affects a very special case only.

matthias-wende-frequenz commented 11 months ago

@cwasicki Do you plan working on this in the next 2 weeks? Otherwise I'd move it to post 1.0.

cwasicki commented 11 months ago

@matthias-wende-frequenz Not planning to work on this, in fact I think this is better assigned someone who is more familiar with the gap algorithm.

cwasicki commented 1 month ago

It seems this is causing problems on one of our sites where the first sample we receive after start-up is None. This results in the moving window to assume that it is completely filled but apart from the latest value contains only garbage data since the ring buffer is initialized with np.empty.

Therefore we should address this for v1.0 because this makes the moving window impossible to use for our forecasting use-cases.

cwasicki commented 1 month ago

Fixed by #1034