Amber: use 15min slots #24084
Merged
Amber: use 15min slots #24084
Conversation
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider sorting the intervals slice by start time before computing minTime to ensure correct slot range and ordering.
- You might want to skip generating slots with no overlap (totalDuration == 0 and no currentPrice) to avoid populating rates with meaningless zero‐value entries.
- The nested logic for computing slot values could be broken out into a helper function for clarity and easier unit testing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider sorting the intervals slice by start time before computing minTime to ensure correct slot range and ordering.
- You might want to skip generating slots with no overlap (totalDuration == 0 and no currentPrice) to avoid populating rates with meaningless zero‐value entries.
- The nested logic for computing slot values could be broken out into a helper function for clarity and easier unit testing.
## Individual Comments
### Comment 1
<location> `tariff/amber.go:147` </location>
<code_context>
+ var currentPrice *float64
+
+ // Find all intervals that overlap with this 15-minute slot
+ for _, interval := range intervals {
+ if interval.end.After(slotStart) && interval.start.Before(slotEnd) {
+ // Calculate overlap duration
</code_context>
<issue_to_address>
**issue (complexity):** Consider sharding intervals into 15-minute buckets in a single pass to avoid nested loops and simplify slot processing.
You can avoid the O(slots × intervals) nested loops by “sharding” each interval into its 15 min buckets in one pass. That way you only iterate slots that actually have data, simplify your logic, and keep exact behavior:
```go
const Slot = 15 * time.Minute
// 1) build a map of per-slot accumulators
type bucket struct {
totalSecs float64
weightedSum float64
current *float64
}
buckets := make(map[time.Time]*bucket)
for _, iv := range intervals {
// truncate start to slot boundary
slot := iv.start.Truncate(Slot)
end := iv.end
for slot.Before(end) {
next := slot.Add(Slot)
// compute overlap [max(slot, iv.start), min(next, iv.end))
s := slot
if iv.start.After(slot) {
s = iv.start
}
e := next
if iv.end.Before(next) {
e = iv.end
}
secs := e.Sub(s).Seconds()
b, ok := buckets[slot]
if !ok {
b = &bucket{}
buckets[slot] = b
}
if iv.isCurrent {
// override any forecast for this slot
b.current = &iv.value
} else {
b.weightedSum += iv.value * secs
b.totalSecs += secs
}
slot = next
}
}
// 2) convert buckets into sorted api.Rates
var data api.Rates
for start, b := range buckets {
var v float64
if b.current != nil {
v = *b.current
} else if b.totalSecs > 0 {
v = b.weightedSum / b.totalSecs
}
data = append(data, api.Rate{Start: start, End: start.Add(Slot), Value: v})
}
// sort by Start if needed, then mergeRates(...)
```
This preserves current‐interval overrides and weighted averaging, but only iterates each interval’s slots once—no full scan over all slots for every interval.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai guide |
Contributor
Reviewer's GuideThe PR refactors rate normalization from fixed 1-hour buckets to flexible SlotDuration shards by replacing the legacy hourly aggregation with a generic bucket sharding approach in a new buildSlotRates helper. Class diagram for Amber rate normalization refactorclassDiagram
class Amber {
+run(done chan error)
+buildSlotRates(intervals)
data: api.Rates
channel: string
}
class bucket {
totalSecs: float64
weightedSum: float64
current: *float64
}
Amber --> bucket : uses for slot aggregation
class api.Rate {
Start: time.Time
End: time.Time
Value: float64
}
class api.Rates {
<<slice of Rate>>
}
Amber --> "api.Rates" : produces
Amber --> "api.Rate" : produces
bucket --> "api.Rate" : produces
Flow diagram for slot-based rate normalizationflowchart TD
A["Amber intervals (5-min/30-min)"] --> B["Sort intervals by start time"]
B --> C["Shard intervals into SlotDuration buckets"]
C --> D["Aggregate weighted average per slot"]
D --> E["Override slot with current interval if present"]
E --> F["Produce sorted api.Rates for each slot"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tariff/amber.go:151` </location>
<code_context>
+ }
+}
+
+// buildSlotRates converts Amber intervals into 15-minute slots using bucket sharding
+// to avoid O(slots × intervals) complexity and only create slots with actual data
+func (t *Amber) buildSlotRates(intervals []struct {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring buildSlotRates to use a flat slice of slot overlaps, sort, and then group in a single pass for clarity and linear workflow.
You can simplify buildSlotRates by collecting all per-slot overlaps into a flat slice, sorting it, then doing a single pass to group by slot. This removes the map lookup noise and separates concerns into three clear steps: expand, sort, reduce.
Example:
```go
func (t *Amber) buildSlotRates(intervals []interval) api.Rates {
type entry struct {
slot time.Time
secs float64
weighted float64
isCurrent bool
value float64
}
// 1) Expand each interval into slot‐sized entries
var entries []entry
for _, iv := range intervals {
for slot := iv.start.Truncate(SlotDuration); slot.Before(iv.end); slot = slot.Add(SlotDuration) {
next := slot.Add(SlotDuration)
start := maxTime(slot, iv.start)
end := minTime(next, iv.end)
secs := end.Sub(start).Seconds()
entries = append(entries, entry{
slot: slot,
secs: secs,
weighted: iv.value * secs,
isCurrent: iv.isCurrent,
value: iv.value,
})
}
}
if len(entries) == 0 {
return nil
}
// 2) Sort by slot
slices.SortFunc(entries, func(a, b entry) int {
return a.slot.Compare(b.slot)
})
// 3) Single pass: group by slot, pick current or compute weighted avg
var rates api.Rates
for i := 0; i < len(entries); {
slot := entries[i].slot
var (
totalSecs float64
totalW float64
curVal *float64
j = i
)
for ; j < len(entries) && entries[j].slot.Equal(slot); j++ {
e := entries[j]
if e.isCurrent {
v := e.value
curVal = &v
} else {
totalSecs += e.secs
totalW += e.weighted
}
}
val := totalW / totalSecs
if curVal != nil {
val = *curVal
}
rates = append(rates, api.Rate{
Start: slot,
End: slot.Add(SlotDuration),
Value: val,
})
i = j
}
return rates
}
// helpers
func minTime(a, b time.Time) time.Time {
if a.Before(b) { return a }
return b
}
func maxTime(a, b time.Time) time.Time {
if a.After(b) { return a }
return b
}
```
— This drops the map, makes the workflow linear (expand→sort→reduce) and keeps the exact same slot-level logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
I'm not sure what this PR really does- switching to 30m slots would have been sufficient. If you think it's good I can merge anyway. |
andig
changed the title
Contributor
Author
|
Hi @andig sorry if it’s not clear! It will work without merging, but because the existing code will normalise to 1-hour slots, then the wrapper normalises that into 15-minute slots, it means the 5-minute resolution from the Amber API becomes less accurate than it would be if it was directly supplied with the 15-minute slots in the first place Let me know if I need to do anything to make to more obvious! |
mfuchs1984
pushed a commit
to mfuchs1984/evcc
that referenced
this pull request
Oct 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Very excited to see #21162 merged!
This updates Amber to normalise to
SlotDurationrather than 1-hour slots. A reminder that Amber can return 5-minute or 30-minute prices (usually 5-minute for the short term and 30-minute for longer, but some accounts will be 30-minute for all due to their smart meter being too old)I considered not doing any normalising (since the wrapper will do this) but we still need the hack to override the current slot with the current price in order to maintain charging session accuracy (please let me know if there's a better way to do this as I could then remove most of this code)