Fix systematic truncation bias in energy calculations - #2668
Conversation
When converting energy values from Watt-seconds to Watt-hours (by dividing by 3600), integer division previously caused truncation toward zero. This resulted in an accumulated loss of precision and an underestimation of historic actual energy generation, which artificially widened the gap between estimated and actual solar production. This patch fixes the calculations to use floating-point division and proper rounding (Convert.ToInt32), ensuring precision is preserved and actual production values are reported correctly. Co-authored-by: pkuehnel <35361981+pkuehnel@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request improves the precision of energy calculations in EnergyDataService.cs by utilizing floating-point division and Convert.ToInt32 for rounding. A review comment identifies a potential issue where missing energy estimates could result in incorrect zero values due to Convert.ToInt32(null) behavior, suggesting a more robust check for null values before performing calculations.
| if (nextMeterValue != default && meterValue != default) | ||
| { | ||
| var energyDifference = Convert.ToInt32((nextMeterValue.EstimatedEnergyWs - meterValue.EstimatedEnergyWs) / 3600); | ||
| var energyDifference = Convert.ToInt32((nextMeterValue.EstimatedEnergyWs - meterValue.EstimatedEnergyWs) / 3600.0); | ||
| createdWh.Add(slicedTimeStamp, energyDifference); | ||
| } |
There was a problem hiding this comment.
While switching to floating-point division and Convert.ToInt32 correctly addresses the truncation bias, the current implementation still treats missing energy estimates (null values) as zero. Since EstimatedEnergyWs is a nullable long?, if either value is missing, the subtraction results in null, and Convert.ToInt32(null) returns 0. This can lead to an underestimation of energy production/consumption when data is incomplete. It is better to only record the difference when both values are present.
if (nextMeterValue?.EstimatedEnergyWs is long nextEnergy && meterValue?.EstimatedEnergyWs is long currentEnergy)
{
var energyDifference = Convert.ToInt32((nextEnergy - currentEnergy) / 3600.0);
createdWh.Add(slicedTimeStamp, energyDifference);
}
Fix systematic truncation bias in energy calculations
When converting energy values from Watt-seconds to Watt-hours (by dividing by 3600), integer division previously caused truncation toward zero. This resulted in an accumulated loss of precision and an underestimation of historic actual energy generation, which artificially widened the gap between estimated and actual solar production. This patch fixes the calculations to use floating-point division and proper rounding (Convert.ToInt32), ensuring precision is preserved and actual production values are reported correctly.
PR created automatically by Jules for task 12971110360911157062 started by @pkuehnel