fix wrong timezone assumptions and adapt tests - #90
Merged
maxnutz merged 5 commits intoJul 9, 2026
Merged
Conversation
2 tasks
Contributor
Reviewer's GuideThis PR fixes incorrect timezone handling by standardizing timestamp localization to UTC+00:00, refines how timestamp-like columns are formatted (including a special path for yearly-aggregated data), updates corresponding tests to expect the new behavior, and bumps the default config’s model name. Flow diagram for updated format_timestamps behaviorflowchart TD
A[Input df with columns] --> B[Create fixed_tz = UTC+00:00]
B --> C[Get cols and idx_name]
C --> D[Parse column labels with pd.to_datetime]
D --> E{All column labels are 4-digit year strings?}
E -->|Yes| F[Convert labels to int years]
F --> G[Set df.columns to integer Index]
G --> H[Return df]
E -->|No| I[Iterate over columns]
I --> J[For each col: check if year-only string]
J --> K[Determine ts from parsed value or year start]
K --> L{ts.tz is not None?}
L -->|Yes| M[Use ts directly as column label]
M --> O[Collect converted labels]
L -->|No| N[Localize ts to fixed_tz]
N --> P{Localization failed?}
P -->|Yes| Q[Set label to pd.NaT and record warning]
P -->|No| R[Use ts_tz as column label]
Q --> O
R --> O
O --> S[Convert cols Index to Python datetime objects]
S --> T[Set df.columns to object Index]
T --> U[Print NaT warnings if any]
U --> V[Return df]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
maxnutz
marked this pull request as ready for review
July 9, 2026 12:02
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new special-case for yearly aggregated data duplicates the year-only detection logic; consider extracting a shared helper or reusing the regex result to avoid running similar checks twice and keep the flow easier to follow.
- Using
printfor warnings informat_timestampsmakes it hard to control output in larger workflows; switching to the standard logging framework would allow callers to configure log levels and handlers more flexibly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new special-case for yearly aggregated data duplicates the year-only detection logic; consider extracting a shared helper or reusing the regex result to avoid running similar checks twice and keep the flow easier to follow.
- Using `print` for warnings in `format_timestamps` makes it hard to control output in larger workflows; switching to the standard logging framework would allow callers to configure log levels and handlers more flexibly.
## Individual Comments
### Comment 1
<location path="pypsa_validation_processing/class_definitions.py" line_range="55-58" />
<code_context>
- for i, col in enumerate(cols):
- is_year_only = isinstance(col, str) and re.match(r"^\d{4}$", col) is not None
- parsed_value = parsed[i]
+ # for yearly aggregated data, use integers as column labels.
+ if all([(isinstance(elem, str) and re.match(r"^\d{4}$", elem)) for elem in cols]):
+ converted_list = [int(col) for col in cols]
+ df.columns = pd.Index(converted_list, name=idx_name)
+ # for non-aggregated data, use timestamps as column labels.
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Behavior for yearly columns now contradicts the function’s timestamp-focused contract.
The yearly-aggregation branch now returns integer column labels instead of tz-aware timestamps, while the function’s name and docstring still promise normalization to UTC+00:00 datetimes. This mismatch can cause subtle bugs for callers that depend on tz-aware columns. Please either keep yearly labels as localized timestamps, or explicitly update the function contract and docstring to document the integer-label behavior so consumers can handle both cases correctly.
</issue_to_address>
### Comment 2
<location path="tests/test_format_timestamps.py" line_range="73-75" />
<code_context>
def test_format_timestamps_preserves_tz_aware_columns():
aware_label = pd.Timestamp(
"2050-01-01 00:00:00",
- tz=datetime.timezone(datetime.timedelta(hours=1)),
+ tz=datetime.timezone(datetime.timedelta(hours=0)),
)
df = pd.DataFrame([[1.0]], columns=[aware_label])
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case that verifies tz-aware columns with a non-UTC offset are preserved unchanged.
The existing test now only covers a tz-aware timestamp with the same UTC+00:00 offset that `format_timestamps` applies. Please add another test using a different timezone (e.g. UTC+02 or `Europe/Berlin`) and assert that `format_timestamps` leaves that timezone intact. This will help catch regressions where tz-aware inputs are incorrectly normalized to UTC.
</issue_to_address>
### Comment 3
<location path="tests/test_format_timestamps.py" line_range="130-133" />
<code_context>
time_columns = [
c
for c in passed_data.columns
- if isinstance(c, (pd.Timestamp, datetime.datetime))
+ if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
assert len(time_columns) == 1
</code_context>
<issue_to_address>
**suggestion (testing):** Extend this test to assert the expected type and timezone/offset of the time column, not just its count.
Including `int` in `time_columns` means this test only verifies that there is one time-like column, not that it has the correct representation. To ensure the timezone fix is covered, the test should assert that for non-aggregated data the time column is a tz-aware UTC timestamp, and for yearly aggregated data it is an `int` year (not a timestamp). Splitting into separate tests for hourly/non-aggregated and yearly/aggregated input and asserting both the type and, where applicable, `utcoffset()`, would make this behavior explicit.
Suggested implementation:
```python
def test_format_timestamps_keeps_unparsable_columns_hourly():
# Non-aggregated/hourly data: time column should be tz-aware UTC timestamp
time_columns = [
c
for c in passed_data.columns
if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
# Ensure the input has exactly one time-like column
assert len(time_columns) == 1
out = format_timestamps(passed_data)
# After formatting, there should still be exactly one time-like column
time_columns_out = [
c
for c in out.columns
if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
assert len(time_columns_out) == 1
time_col = time_columns_out[0]
# For non-aggregated data, the time column must be a tz-aware UTC timestamp
assert isinstance(time_col, (pd.Timestamp, datetime.datetime))
assert time_col.tzinfo is not None
assert time_col.utcoffset() == datetime.timedelta(hours=0)
def test_format_timestamps_keeps_unparsable_columns_yearly():
# Yearly/aggregated data: time column should be an integer year
time_columns = [
c
for c in yearly_passed_data.columns
if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
# Ensure the input has exactly one time-like column
assert len(time_columns) == 1
out = format_timestamps(yearly_passed_data)
# After formatting, there should still be exactly one time-like column
time_columns_out = [
c
for c in out.columns
if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
assert len(time_columns_out) == 1
time_col = time_columns_out[0]
# For yearly aggregated data, the time column must be an integer year
assert isinstance(time_col, int)
```
To fully implement the suggestion, you will likely need to:
1. Ensure `yearly_passed_data` is defined as a fixture or test data in this module (or imported) representing yearly/aggregated input, analogous to `passed_data` but with a yearly time column.
2. Confirm that `format_timestamps` actually produces an integer year for yearly/aggregated input; if it currently returns timestamps, update the implementation accordingly or adjust the fixture so it exercises the intended code path.
3. If there are multiple types of aggregated inputs (e.g. monthly, daily), consider adding similar explicit tests for those to keep the behavior clear and covered.
</issue_to_address>
### Comment 4
<location path="tests/test_format_timestamps.py" line_range="136" />
<code_context>
+ if isinstance(c, (pd.Timestamp, datetime.datetime, int))
]
assert len(time_columns) == 1
- assert time_columns[0].utcoffset() == datetime.timedelta(hours=1)
</code_context>
<issue_to_address>
**issue (testing):** Replace the removed utcoffset assertion with one that validates the new UTC+00:00 behavior where applicable.
The original test ensured tz-aware timestamps with a +1 hour offset; with that removed, we should still assert the new UTC+00:00 behavior for non-aggregated timestamps. If `structure_pyam_from_pandas` now produces UTC tz-aware hourly data, please add an assertion on `time_columns[0].utcoffset()` for that case. If aggregated data intentionally uses naïve integer years instead, add a separate test for that path so both behaviors and the timezone normalization fix are clearly covered.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…s-to-wrong-timesteps-on-scenario-explorer
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
1 participant
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.
Short description of this pull request
changes the timezone assumptions from hours+1 to hours+0, as scenario explorer itself creates respective timezone data leading to "double counting".
Checklist
Before asking for review, please make shure, the following steps are completed (whenever possible):
Sourcery-Bot starts to review your pull request, whenever it is created. This may take some time. After having finished these steps, please request for review in the Pull Request.
Summary by Sourcery
Adjust timestamp handling to avoid incorrect timezone offsets and improve support for yearly aggregated data.
Bug Fixes:
Enhancements:
Tests: