fix(resampler): make interval grouping consistent regardless of first_timestamp#75
Open
phillip-wenig-frequenz wants to merge 2 commits into
Conversation
phillip-wenig-frequenz
force-pushed
the
fix/first-timestamp-interval-grouping
branch
from
2f5df2e to
b939daf
Compare
…_timestamp The `first_timestamp` parameter was incorrectly affecting both output timestamp labeling and interval grouping semantics. When `first_timestamp=false`, intervals used `(start, end]` instead of `[start, end)`, causing samples at exact interval boundaries to be excluded. This fix makes interval grouping consistently use `[start, end)` regardless of `first_timestamp` value. The parameter now only controls the output timestamp labeling as documented: `true` labels with the interval start, `false` with the interval end. Changes include: - Simplified boundary checking functions to remove first_timestamp logic - Updated documentation to clarify the parameter's sole purpose - Fixed test data to start at t=0 to match README example - Added explicit test for first_timestamp=false behavior - Updated RELEASE_NOTES with bug fix details Signed-off-by: Phillip Wenig <phillip.wenig@frequenz.com>
Add tests to strengthen coverage of the first_timestamp fix: Rust tests (src/tests.rs): - test_first_timestamp_same_values: Verifies both settings produce identical aggregated values, differing only in output timestamps - test_sample_at_interval_boundary: Confirms sample at t=5 goes to interval [5, 10), validating [start, end) semantics - test_data_starting_mid_interval: Tests correct behavior when data doesn't start at interval boundary Python tests (tests/test_resampler.py): - Updated all tests to start data at t=0 to match README example - Fixed expected values to match correct [start, end) interval semantics Signed-off-by: Phillip Wenig <phillip.wenig@frequenz.com>
phillip-wenig-frequenz
force-pushed
the
fix/first-timestamp-interval-grouping
branch
from
b939daf to
4f191e6
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where the first_timestamp parameter was incorrectly affecting interval grouping logic instead of only controlling output timestamp labeling. The fix ensures intervals are consistently [start, end) regardless of the first_timestamp value, resolving issue #74 where the first sample at t=0 was being excluded when first_timestamp=false.
Changes:
- Simplified
is_left_of_buffer_edgeandis_right_of_buffer_edgefunctions to remove dependency onfirst_timestampparameter - Updated all Rust and Python tests to start data at
t=0to match README examples - Added comprehensive edge case tests (boundary samples, mid-interval data start, first_timestamp consistency)
- Updated documentation in Rust code, Python stubs, and RELEASE_NOTES.md
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/resampler.rs | Core fix: removed first_timestamp parameter from boundary functions; updated documentation to clarify parameter purpose |
| src/tests.rs | Updated existing tests to start at t=0; added 4 new comprehensive edge case tests |
| src/lib.rs | Updated README example to start data at t=0 |
| tests/test_resampler.py | Updated all Python tests to start at t=0 and added clarifying comments |
| frequenz/resampling/_rust_backend.pyi | Updated docstring to clarify first_timestamp only affects output labeling |
| RELEASE_NOTES.md | Added bug fix entry describing the change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
phillip-wenig-frequenz
added a commit
to phillip-wenig-frequenz/frequenz-resampling-rs
that referenced
this pull request
Apr 21, 2026
…enz-floss#78) ## Summary Add a new `resample()` convenience function available in both Rust and Python APIs, allowing users to resample data in a single call without needing to instantiate and manage a `Resampler` instance. ### Rust API ```rust use frequenz_resampling::{resample, ResamplingFunction, SimpleSample}; let result = resample(&data, TimeDelta::seconds(5), ResamplingFunction::Average, true); ``` ### Python API ```python from frequenz.resampling import resample, ResamplingFunction result = resample(data, timedelta(seconds=5), ResamplingFunction.Average) ``` ## Changes - Added `resample()` function in Rust (`src/lib.rs`) with `SimpleSample` type - Added `resample()` Python binding (`src/python.rs`) - Made `epoch_align()` public to support the new functionality - Updated README with "One-Shot Resampling" and "Stateful Resampling" sections - Updated type stubs with `Sequence` input type for flexibility - Added comprehensive test coverage in both Rust and Python - Updated RELEASE_NOTES.md depends on frequenz-floss#75
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
first_timestampparameter to only affect output timestamp labeling, not interval groupingfirst_timestamp=falsewould shift interval boundaries from[start, end)to(start, end], causing the first sample att=0to be excluded[start, end)regardless offirst_timestampvalueChanges
is_left_of_buffer_edgeandis_right_of_buffer_edgefunctions to use consistent boundary logicfirst_timestamponly controls output timestamp labelingt=0matching the README exampleFixes #74