-
-
Notifications
You must be signed in to change notification settings - Fork 94
Fix bug in previous days gap filling, fix crash with tried_list #3236
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the historical load data gap filling logic where days beyond the available data age were incorrectly mapped to earlier days due to clamping in the use_days calculation.
Changes:
- Fixed the loop in
previous_days_modal_filterto compute sums for all days up to the maximum requested, rather than clamping toload_minutes_age - Changed
use_dayscalculation frommax(min(days, self.load_minutes_age), 1)tomax(days, 1)to allow proper detection of days with no data - Added calculation of average non-zero day consumption to use as fallback instead of hardcoded 24 kWh
- Added standalone test file to verify the bug fix
- Reset
tried_listcache between optimization passes in plan.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/predbat/fetch.py | Core bug fix - Changed loop to iterate through all days and compute average non-zero consumption for gap filling |
| coverage/test_modal_filter.py | Added standalone test to verify the fix (references missing YAML file) |
| apps/predbat/plan.py | Reset optimization cache between passes (appears unrelated to gap filling) |
apps/predbat/fetch.py
Outdated
| for days in range(max(max(days_list), self.load_minutes_age) + 1): | ||
| use_days = max(days, 1) |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop range has two issues: (1) it starts from 0, but days=0 and days=1 both result in use_days=1 and full_days=0, causing redundant computation; (2) if days_list is empty, max(days_list) will raise ValueError. The loop should start from 1 and handle empty lists: for days in range(1, max(max(days_list, default=1), self.load_minutes_age) + 1). This ensures historical days are properly 1-indexed and handles edge cases safely.
apps/predbat/fetch.py
Outdated
| for days in range(max(max(days_list), self.load_minutes_age) + 1): | ||
| use_days = max(days, 1) |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop iterates from 0, but days=0 and days=1 both result in use_days=1 and full_days=0, causing the same data to be computed twice. Historical days should be 1-indexed (day 1 = yesterday). The loop should start from 1 instead: for days in range(1, max(max(days_list), self.load_minutes_age) + 1). This will avoid redundant computation and ensure sum_all_days is calculated correctly.
apps/predbat/fetch.py
Outdated
| min_sum = dp2(sum_day) | ||
| sum_all_days[days] = dp2(sum_day) | ||
|
|
||
| # Work out the highest non-zero day load |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Work out the highest non-zero day load" but the code actually computes the average of all non-zero days, not the highest value. The comment should be corrected to "Work out the average non-zero day load" to accurately describe what the code does.
apps/predbat/plan.py
Outdated
| ) | ||
|
|
||
| self.plan_write_debug(debug_mode, "plan_pre_levels.html", self.pv_forecast_minute_step, self.pv_forecast_minute10_step, self.load_minutes_step, self.load_minutes_step10, self.end_record) | ||
| tried_list = {} |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears unrelated to the "previous days gap filling" bug fix described in the PR title. Resetting tried_list here clears the optimization cache between the initial optimization pass and the region-based optimization passes. While this may be a valid optimization improvement, bundling unrelated changes in a single PR makes it harder to review and understand the purpose of each change. Consider either documenting why this is related to gap filling, or moving it to a separate PR.
| # Sensible threshold to ignore zero days | ||
| if day_sum > 2.5: |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The threshold of 2.5 kWh for determining zero days seems arbitrary and could be problematic for very low consumption households or edge cases. Consider making this threshold configurable or basing it on a percentage of the average consumption rather than a fixed value. For example, a household with naturally low consumption (e.g., 5 kWh/day) would still be counted, but a household with a partial day of 3 kWh due to data issues would also be counted.
No description provided.