Repository navigation
fix(history): keep unseen optional PV out of load training - #1759
Merged
Merged
Conversation
History leaves an optional PV source out of Home use when it has never reported, as a hybrid may have no panels. The forecast requires it, and both load models now train on history intervals, so 2 kW of unseen PV could become a 1 kW label for a 3 kW house (Codex review of #1754). History options now carry the optional flows. While one has no live reading, the snapshot is marked EstimatedLoad: Home use is unchanged and training skips it. Past civil dusk the PV is 0 W by physics, so the night still trains. Forecast revisions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Problem
Codex's adversarial review of #1752–#1755 (high): History leaves an optional PV source out of Home use when it has never reported, because a hybrid inverter may have no panels (
historyMeasurementOptionsdrops it;forecastMeasurementOptionsrequires it). Since #1754 both load models train on history intervals, so such a quarter could become a training label. Example: 1 kW grid import plus 2 kW of unseen PV becomes a 1 kW label for a 3 kW house. The old forecast collector never trained on it.Change
telemetry.ForecastOptions.OptionalFlowscarries the flows History does not require but a training label does.historyMeasurementOptionsfills it. The field isomitempty, and only the forecastOptionsare hashed into the learning revision, so no site loses learning.optionalPVUnread(beside fix(history): count unknown PV as 0 W after sunset #1755's dark-PV rule) marks a snapshotEstimatedLoadwhile an optional PV source has no live reading. Home use is unchanged; training skips the interval, per the two-layer contract.Evidence
TestHistoryOptionsKeepOptionalPVForTraining: History does not require the optional PV, carries it inOptionalFlows, and the forecast options' JSON (the revision input) has no new key.TestOptionalPVUnreadKeepsDaytimeOutOfTraining: day and twilight are marked, past civil dusk is not, a night without a pin is, and a reporting PV is not; nothing changes what History shows.go test ./cmd/ftw ./internal/state ./internal/telemetry ./internal/loadmodel ./internal/api: all pass. Telemetry-read guard: none added.The other Codex finding (a rejected held value can return) is being fixed separately in the history writer.
🤖 Generated with Claude Code