Fix slack distribution outerloop incorrect stable status #1157
Conversation
jeandemanged
changed the title
jeandemanged
changed the title
| final boolean movedBuses = initialP.entrySet().stream() | ||
| .anyMatch(e -> Math.abs(e.getKey().getTargetP() - e.getValue()) > P_RESIDUE_EPS); | ||
| final boolean movedBuses = Math.abs(initialP.entrySet().stream() | ||
| .mapToDouble(e -> e.getKey().getTargetP() - e.getValue()).sum()) > P_RESIDUE_EPS; | ||
|
|
There was a problem hiding this comment.
If unlucky (mismatch very close to P_RESIDUE_EPS, and roundings you could be below the criteria)
Maybe > P_RESIDUE_EPS*0.9 to avoid rounding effects ?
| final boolean movedBuses = initialP.entrySet().stream() | ||
| .anyMatch(e -> Math.abs(e.getKey().getTargetP() - e.getValue()) > P_RESIDUE_EPS); | ||
| final boolean movedBuses = Math.abs(initialP.entrySet().stream() | ||
| .mapToDouble(e -> e.getKey().getTargetP() - e.getValue()).sum()) > P_RESIDUE_EPS; |
There was a problem hiding this comment.
Isn't it better to sum the abs values instead of doing the abs value of the sum ?
There was a problem hiding this comment.
in today's distribution all elements go always in the same direction, therefore we can spare a few cpu cycles with a single Math.abs
But maybe we should indeed put abs on each element to avoid surprises in the future in case we add more fancy distributions, no strong opinion here ...
There was a problem hiding this comment.
Oh yes that's true. No strong opinion neither, these cpu cycles may not be perceptible, but it can stay as it is.
There was a problem hiding this comment.
in the end I change to what you suggested, imagine in the future we do fancy distribution on both loads and generators...
|
Signed-off-by: Damien Jeandemange <damien.jeandemange@artelys.com> (cherry picked from commit c7f69ad)
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Regression introduced in the #1148 refactoring.The root problem is not really the refactoring, actually the code before the refactor was hiding a bug. The real problem is the computation of the
movedBusesindicator inActivePowerDistribution.In the following situation:
then each element moves by only 0.000015 MW, below epsilon threshold. Consequently
ActivePowerDistributionreports that no bus has moved.Next Consequences:
What is the new behavior (if this is a feature change)?
movedBusesindicator inActivePowerDistributioncorrectly accounts for many many small changes, by summing them instead of checking individual values.Does this PR introduce a breaking change or deprecate an API?