Session: add reference price and CO2#28712
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
AverageRatehelper currently computes a simple arithmetic mean of all overlapping rates, which biases the result if the underlying rate periods have different durations; consider making this a duration-weighted average over the [now, now+d) interval instead. AverageRatehardcodestime.Now()inside the function, which makes behavior time-dependent and harder to test; passing the start time as a parameter (or injecting a clock) would make the function more predictable and reusable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `AverageRate` helper currently computes a simple arithmetic mean of all overlapping rates, which biases the result if the underlying rate periods have different durations; consider making this a duration-weighted average over the [now, now+d) interval instead.
- `AverageRate` hardcodes `time.Now()` inside the function, which makes behavior time-dependent and harder to test; passing the start time as a parameter (or injecting a clock) would make the function more predictable and reusable.
## Individual Comments
### Comment 1
<location path="tariff/tariffs.go" line_range="47-56" />
<code_context>
return rr
}
+// AverageRate returns the arithmetic mean of rates in [now, now+d), or nil if unavailable.
+func AverageRate(t api.Tariff, d time.Duration) *float64 {
+ rr := Rates(t)
+ if rr == nil {
+ return nil
+ }
+
+ now := time.Now()
+ end := now.Add(d)
+
+ var sum float64
+ var count int
+ for _, r := range rr {
+ if r.Start.Before(end) && r.End.After(now) {
+ sum += r.Value
+ count++
+ }
+ }
+
+ if count == 0 {
+ return nil
+ }
+
+ avg := sum / float64(count)
+ return &avg
+}
</code_context>
<issue_to_address>
**suggestion:** Consider weighting the average by time instead of counting overlapping rate entries equally.
Right now each overlapping rate contributes equally to the mean, regardless of how long it applies within `[now, now+d)`. If rate periods can vary a lot in length, this can significantly skew the result (e.g. brief spikes vs long stable periods). You may want to compute a time‑weighted average instead: for each overlapping rate, accumulate `r.Value * overlapDuration` and then divide the total by the sum of all overlap durations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
andig
reviewed
Apr 6, 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.
addresses #11160
Snapshot average grid price and CO2 intensity (next 24h) when a charging session is created.
Adds referencePricePerKWh and referenceCo2PerKWh to the session DB schema and JSON/CSV response. This provides stable reference values for future savings calculations instead of relying on volatile forecast data.
Existing sessions get NULL via GORM auto-migration.
Next steps: (separate PRs)