Repository navigation
fix(history): reconstruct car slots into today-so-far, not just yesterday (#5004) - #5048
Merged
Merged
Conversation
…rday (#5004) The #5004 fix (#5025) widened yesterday_load_step to cover today-so-far - calculate_yesterday deliberately extends the History view's axis to 24*60 + minutes_now so it can show today alongside yesterday - but left yesterday_reconstruct_car_slots' own end_record capped at the bare 24*60. The reconstruction loop (for start_minute in range(0, end_record, ...)) never reached past yesterday midnight, so any car session that happened today stayed in the raw, unsubtracted load band until the next day's run rolled it into the now-covered "yesterday" range. Reported by tdbegley28 on the closed issue: car charging correctly separates out for every prior day, but today's stays as plain load until midnight. Pass end_record + minutes_now to the reconstruction call, matching the axis yesterday_load_step already covers, rather than the yesterday-only cap. Verified directly against yesterday_reconstruct_car_slots(): an identical fixture with the session moved from yesterday 01:00 to today 01:00 went from correctly reconstructing a slot to reconstructing nothing at all, at the unfixed end_record; fixed, both place the slot and subtract the load. Two new tests pin this - one exercising the function directly at both widths (5i), one confirming calculate_yesterday's own call site passes the widened value so a future regression there is caught even if 5i alone would still look correct in isolation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One critical reconstruction issue and one moderate test issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes History car-slot reconstruction so today-so-far charging is separated from household load, with regression coverage.
Changes:
- Extends reconstruction through today-so-far.
- Adds direct and call-site regression tests.
File summaries
| File | Summary |
|---|---|
apps/predbat/output.py |
Widens reconstruction range. A critical issue (2 votes) remains for unaligned minutes_now causing incorrect index wrapping. |
apps/predbat/tests/test_calculate_yesterday.py |
Adds regression coverage. A moderate issue (1 vote) remains because the expected value can mask failure to restore the clock. |
Review details
Suppressed comments (1)
apps/predbat/tests/test_calculate_yesterday.py:1134
- This expected value is derived from
my_predbat.minutes_nowaftercalculate_yesterday()returns, even though that method temporarily rewrites the attribute to 0. If a future regression both passes the bareend_recordand fails to restore the clock,captured_end_record[0]and this expected value would both be 1440, so the call-site regression would pass unnoticed. Base the expectation on thenow_utcfixture (or captureminutes_nowbefore the call) instead.
expected = 24 * 60 + my_predbat.minutes_now
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…end_record (Copilot review on #5048) minutes_now is only rounded to PREDICT_STEP (5 minutes), not to plan_interval_minutes (30), so end_record = minutes_now + 24*60 (the real "now" on the widened axis) is not generally a bucket boundary. The reconstruction loop's inner scan (range(start_minute, start_minute + plan_interval_minutes)) ran to the full bucket width regardless, so the last bucket - the one still in progress - could scan a few minutes past the real "now". get_historical_base()'s minute_previous = base_minutes - minute goes negative for those extra minutes, and get_from_incrementing() silently wraps a negative index by +24*60 - reading yesterday's data at roughly the same clock time instead of "nothing has happened yet", and miscounting it into today's still-in-progress bucket. The exact ghost-slot mechanism #5004 was fixed for elsewhere in this same function. Confirmed numerically before fixing: with minutes_now=365 (06:05, 5-aligned but not 30-aligned), the unclamped inner loop for the last bucket [1800, 1830) read wrapped indices 1416-1439 - yesterday's data - contributing 3.0 kWh from a session placed there, versus 0.0 once clamped to end_record. Verified with mutation testing: the new test (5j) fails with the exact reverted-loop bound, reproducing a spurious slot in the in-progress final bucket; passes with the clamp restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ate_yesterday can fake it (Copilot review on #5048) The end_record assertion compared against my_predbat.minutes_now read AFTER calculate_yesterday() returns, but that call fakes self.minutes_now to 0 for its duration and only restores it near the end. A regression breaking both the call site (passing bare end_record) and the restore simultaneously would leave the captured value and the after-the-fact expected value drifting identically, passing vacuously - confirmed by reverting both together. Capture minutes_now up front instead, before it can be faked, and assert the wrapper is actually called with that real value too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
(Posted by Rik; written by Claude.) Addressed a suppressed Copilot finding on the test added for the clamped-bucket fix ( The test's Fixed by capturing the real Mutation-tested: reverted the double-mutation (bare |
springfall2008
approved these changes
Sep 12, 2026
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.
Posted by Claude on behalf of @chalfontchubby.
Fixes #5004.
Problem
The #5004 fix (#5025) correctly widened
yesterday_load_stepto cover today-so-far -calculate_yesterday()deliberately extends the History view's axis to24*60 + minutes_nowso it can show today alongside yesterday - but leftyesterday_reconstruct_car_slots()'s ownend_recordcapped at the bare24*60.The reconstruction loop (
for start_minute in range(0, end_record, ...)) never reached past yesterday midnight, so any car session that happened today stayed in the raw, unsubtracted load band until the next day's run rolled it into the now-covered "yesterday" range. Reported by @tdbegley28 on the closed issue: car charging correctly separates out for every prior day, but today's stays as plain load until midnight.Fix
Pass
end_record + minutes_nowto the reconstruction call, matching the axisyesterday_load_stepalready covers, rather than the yesterday-only cap.Verification
Reproduced directly against
yesterday_reconstruct_car_slots(): an identical fixture with the session moved from yesterday 01:00 to today 01:00 went from correctly reconstructing a slot to reconstructing nothing at all, at the unfixedend_record; fixed, both place the slot and subtract the load.Two new tests pin this:
_test_reconstruct_car_slots)calculate_yesterday()'s own call site passes the widened value, so a future regression at the call site is caught even if 5i alone would still look correct in isolationBoth verified to fail against the unpatched call site and pass with the fix.
./run_all --quickgreen./run_pre_commitgreen🤖 Generated with Claude Code