Skip to content

Bugfix nat in buddy check #614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vergauwenthomas merged 2 commits into from
Feb 17, 2026
Merged

Bugfix nat in buddy check #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b29e3c
remove the floor and ceil in synchronizing timeseries in buddy check
vergauwenthomas Feb 17, 2026
d25cc6c
minor version bump
vergauwenthomas Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ requires = ["poetry-core"]

[project]
name = "MetObs-toolkit"
version = "1.0.0"
version = "1.0.1"
license = "LICENSE"
authors = [{name = "Thomas Vergauwen", email = "thomas.vergauwen@ugent.be"}]
description = "A Meteorological observations toolkit for scientists"
Expand Down
8 changes: 4 additions & 4 deletions src/metobs_toolkit/qc_collection/buddy_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ def synchronize_series(


* freq: the highest frequency present in the input series
* origin: the earliest timestamp found, rounded down by the freq
* closing: the latest timestamp found, rounded up by the freq.
* origin: the earliest timestamp found across all input series
* closing: the latest timestamp found across all input series.

Comment on lines +34 to 35
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring now states that closing is “the latest timestamp found across all input series”, but pd.date_range(start=origin, end=closing, freq=trg_freq) will not necessarily include closing unless it lies exactly on the generated grid. This can make the documented definition of the target timestamps inaccurate and may hide edge cases where the last observations fall outside the generated index/tolerance window. Either clarify the wording (upper bound, not guaranteed included) or adjust the index generation to guarantee inclusion of the final boundary.

Suggested change
* closing: the latest timestamp found across all input series.
* closing: an upper time bound given by the latest timestamp found
across all input series (not necessarily itself included in the
generated target index).

Copilot uses AI. Check for mistakes.
Parameters
----------
Expand All @@ -55,8 +55,8 @@ def synchronize_series(
trg_freq = min(frequencies)

# find origin and closing timestamp (earliest/latest)
origin = min([s.index.min() for s in series_list]).floor(trg_freq)
closing = max([s.index.max() for s in series_list]).ceil(trg_freq)
origin = min([s.index.min() for s in series_list])
closing = max([s.index.max() for s in series_list])
Comment on lines +58 to +59
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change alters the time-synchronization behavior in a way that can be sensitive to small instantaneous_tolerance values (default 4 minutes). There are buddy check tests, but none appear to cover a regression where one series starts/ends off the nominal trg_freq grid (or where closing is not on-grid), which is exactly where this bugfix/behavior change matters. Adding a focused unit/regression test for synchronize_series (or a buddy_check integration test) with misaligned start/end timestamps would help prevent future regressions.

Copilot uses AI. Check for mistakes.

# Create target datetime axes
target_dt = pd.date_range(start=origin, end=closing, freq=trg_freq)
Comment on lines 61 to 62
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the raw earliest/latest timestamps as start/end for pd.date_range can create a target axis that is not aligned to trg_freq (it becomes anchored on the earliest timestamp). With the default instantaneous_tolerance of 4 minutes, this can cause systematic timestamp shifts or even drop otherwise valid observations when stations are on a different (but regular) grid. Consider keeping the target axis aligned to trg_freq (e.g., floor/ceil using a proper pandas offset) while still computing/reporting the unrounded min/max separately if needed.

Suggested change
# Create target datetime axes
target_dt = pd.date_range(start=origin, end=closing, freq=trg_freq)
# Align the target datetime axis to the inferred frequency by
# flooring/ceiling the global min/max timestamps to trg_freq.
origin_aligned = origin.floor(trg_freq)
closing_aligned = closing.ceil(trg_freq)
# Create target datetime axes
target_dt = pd.date_range(start=origin_aligned, end=closing_aligned, freq=trg_freq)

Copilot uses AI. Check for mistakes.
Expand Down
2 changes: 1 addition & 1 deletion src/metobs_toolkit/settings_collection/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = "1.0.0"
__version__ = "1.0.1"