Fix off-by-one in `OrderedRingBuffer.count_covered` and add consistency test #1343

malteschaaf · 2026-01-12T15:38:25Z

Summary

This PR fixes a subtle off-by-one issue in the OrderedRingBuffer.count_covered method and adds a test to ensure window count consistency in MovingWindow.

Changes

Fix count_covered calculation
- Switched from floating-point division to integer timedelta division (timedelta // timedelta).
- This eliminates rounding errors in sample count calculations and ensures accurate coverage for all window sizes and sampling periods.
Add test_moving_window_length
- New test verifies that the moving window length is consistent without resampling.
- Confirms that the count of covered samples matches the expected window capacity.
- Helps prevent regressions related to off-by-one errors in the future.

Motivation

Floating-point division could produce off-by-one errors for certain window sizes and sampling periods, causing count_covered to be inaccurate. Switching to integer-based timedelta division ensures deterministic and exact counts. The new test provides coverage to catch similar issues.

llucax

Thanks a lot! LGTM, only a couple of minor comments.

Also the CI is not passing, it is weird because the issues look unrelated to this PR, but maybe you can fix them (removing the type: ignore should be enough, maybe mypy or numpy got some hidden improvement in some patch release).

nox > mypy 
src/frequenz/sdk/timeseries/_periodic_feature_extractor.py:412: error: Unused "type: ignore" comment  [unused-ignore]
benchmarks/timeseries/periodic_feature_extractor.py:75: error: Unused "type: ignore" comment  [unused-ignore]

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

tests/timeseries/test_moving_window.py

RELEASE_NOTES.md

llucax · 2026-01-14T09:24:23Z

Very weird, now tests are failing in some apparently unrelated tests (I don't think these tests use the moving window). 🤔

llucax · 2026-01-14T09:31:03Z

I just re-run the tests and they fail the same, so that's good news (I was more scare about this being a flaky failure). I guess the next step would be to verify tests pass without these changes. If the failure is consistent, then probably these changes are somehow affecting those tests and we need to find why and fix it.

@malteschaaf are the tests passing locally?

malteschaaf · 2026-01-14T09:37:57Z

@malteschaaf are the tests passing locally?

The tests fail locally as well but no matter the change (they fail with and without any of my changes)

llucax · 2026-01-14T11:33:23Z

OK, this is definitely some indirect dependency bump issue. Locally using nox -s pytest_min works but nox -s pytest_max doesn't. Probably the best way to approach this is to see in the CI what was the last successful build and see the output for the pip freeze command to get a list of dependencies. Then do the same in one failing build and then compare the two to see what are the differences and find which one is breaking stuff.

@malteschaaf do you think you can take care of this?

malteschaaf · 2026-01-14T12:25:55Z

Seems like frequenz-microgrid-component-graph v0.3.4 caused this

I'm not sure how to proceed from here though...
I feel like just pushing a commit restricting the max version of frequenz-microgrid-component-graph isn't the best way.

llucax · 2026-01-15T11:23:39Z

I feel like just pushing a commit restricting the max version of frequenz-microgrid-component-graph isn't the best way.

I'll have a look to see if there is anything obvious in the new version that is wrong. Thanks for the research!

llucax · 2026-01-15T11:30:03Z

OK, it looks like v0.3.4 introduced breaking changes (coming from the rust library v0.4.0):

Upgrading

The PV, battery, CHP, EV and Wind Turbine formulas now prefer meters as the primary components and fallback to inverters or other corresponding components only when the meters are not available.

This makes a big difference in performance when there are multiple PV inverters behind a single meter, for example.

This behaviour can be changed with the newly introduced meter preference config flags.

Consumer formulas don't consider phantom loads by default anymore. The original behaviour is still available through a config flag.

I will mark that release as broken and wait for Sahas to come back to see how we move forward (we probably need to re-release it as v0.4.0 and then update the SDK tests to pass using the new logic).

Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

…y-one Previously, floating-point division could produce off-by-one errors for count_covered due to rounding issues (e.g., 1.0 // 0.1 == 9). This change uses `timedelta // timedelta`, which operates on integer nanoseconds, ensuring the returned sample count is exact and preventing count_valid from exceeding count_covered during initial window fill. Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

Adds a new test `test_moving_window_length` that checks the moving window length without resampling. The test ensures that `count_valid` never exceeds `count_covered` during the initial fill and that the final count matches the expected window capacity. This helps prevent regressions related to off-by-one errors in window count calculations. Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

…ing changes - Updated the maximum version of "frequenz-microgrid-component-graph" from `< 0.3.4` to `<= 0.3.3`. - This change ensures compatibility, as version 0.3.4 introduces breaking changes. Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

llucax · 2026-01-15T13:21:47Z

pyproject.toml

  # (plugins.mkdocstrings.handlers.python.import)
  "frequenz-client-microgrid >= 0.18.1, < 0.19.0",
-  "frequenz-microgrid-component-graph >= 0.3.2, < 0.4",
+  "frequenz-microgrid-component-graph >= 0.3.2, < 0.3.4",


This should have not be committed 😭

We are finally not going this route. I made a follow-up PR.

Disable tests that need adapting to the new component graph #1346

No worries and keep the contributions coming!

malteschaaf requested a review from a team as a code owner January 12, 2026 15:38

malteschaaf requested review from llucax and removed request for a team January 12, 2026 15:38

github-project-automation bot added this to Python SDK Roadmap Jan 12, 2026

github-project-automation bot moved this to To do in Python SDK Roadmap Jan 12, 2026

github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Jan 12, 2026

cwasicki previously approved these changes Jan 12, 2026

View reviewed changes

github-project-automation bot moved this from To do to Review approved in Python SDK Roadmap Jan 12, 2026

llucax reviewed Jan 13, 2026

View reviewed changes

src/frequenz/sdk/timeseries/_ringbuffer/buffer.py Outdated Show resolved Hide resolved

tests/timeseries/test_moving_window.py Outdated Show resolved Hide resolved

RELEASE_NOTES.md Outdated Show resolved Hide resolved

malteschaaf dismissed cwasicki’s stale review via 78ed90d January 13, 2026 09:07

malteschaaf force-pushed the fix_count_covered branch from ac1d92d to 78ed90d Compare January 13, 2026 09:07

malteschaaf requested review from cwasicki and llucax January 13, 2026 09:13

cwasicki previously approved these changes Jan 13, 2026

View reviewed changes

llucax mentioned this pull request Jan 14, 2026

Validate component IDs when creating component pools #1340

Open

malteschaaf dismissed cwasicki’s stale review via 7e9fbb9 January 14, 2026 12:25

github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Jan 14, 2026

cwasicki approved these changes Jan 14, 2026

View reviewed changes

malteschaaf added 3 commits January 15, 2026 12:58

Reset release notes

f3beb28

Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

malteschaaf added 3 commits January 15, 2026 12:58

Remove unused mypy type ignores

e08e266

Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

Update release notes

85746b0

Signed-off-by: Malte Schaaf <malte.schaaf@frequenz.com>

malteschaaf force-pushed the fix_count_covered branch from 7e9fbb9 to 039de13 Compare January 15, 2026 11:59

cwasicki added this pull request to the merge queue Jan 15, 2026

Merged via the queue into frequenz-floss:v1.x.x with commit 5d80a01 Jan 15, 2026
5 checks passed

github-project-automation bot moved this from Review approved to Done in Python SDK Roadmap Jan 15, 2026

llucax reviewed Jan 15, 2026

View reviewed changes

Fix off-by-one in OrderedRingBuffer.count_covered and add consistency test #1343

Fix off-by-one in OrderedRingBuffer.count_covered and add consistency test #1343

Uh oh!

Conversation

malteschaaf commented Jan 12, 2026

Summary

Changes

Motivation

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llucax commented Jan 14, 2026

Uh oh!

llucax commented Jan 14, 2026

Uh oh!

malteschaaf commented Jan 14, 2026

Uh oh!

llucax commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malteschaaf commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llucax commented Jan 15, 2026

Uh oh!

llucax commented Jan 15, 2026

Upgrading

Uh oh!

Uh oh!

llucax Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

llucax Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

malteschaaf Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

llucax Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix off-by-one in `OrderedRingBuffer.count_covered` and add consistency test #1343

Fix off-by-one in `OrderedRingBuffer.count_covered` and add consistency test #1343

llucax commented Jan 14, 2026 •

edited

Loading

malteschaaf commented Jan 14, 2026 •

edited

Loading