Fix proportional scalable infinite loop

[jeandemanged]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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?

Infinite loop in proportional scalable under the following condition:

• asking a little bit more power than the full available power range over all scalables
• scalable percentages are low enough so that the difference of asked power minus done power on one scalable falls below the 0.01 MW epsilon

In this case the iteration percentage of the scalable is never set to zero, there is remaining asked power, and the scaling is stuck in an infinite loop.

May sound like a marginal case, but was observed on real data. On large networks a given load may have a small percentage.

What is the new behavior (if this is a feature change)?
We set iteration percentage to zero when a scalable is saturated, in addition to the existing condition checking that not all asked power has been done.

Does this PR introduce a breaking change or deprecate an API?

• No

Other information:

[phiedw]

Well spotted for the issue!
However I don't think the fix is the correct one.
As you can see with the example I added, there can be other similar cases which cause infinite looping (which are not fixed by this pull request)

[phiedw]

What should the behaviour be in the following case? (maybe too much of a marginal case for us to worry about)

Initially load1 is at 0MW, load2 and load3 are at 100MW, and the coefficients on the different loads are ~100, 1e-5 and 1e-5.

If we ask for a 100MW reduction, with the new code, we will scale by 2e-3MW at the first iteration and will exit the loop.
Before, we would have scaled by 2e-3MW at the first iteration, renormalized the coefficients (since load1 is saturated), and then at the second iteration, scaled by 50MW on both loads 2 and 3.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[phiedw]

Thank you very much for this pull request

[jeandemanged]

What should the behaviour be in the following case? (maybe too much of a marginal case for us to worry about)

Initially load1 is at 0MW, load2 and load3 are at 100MW, and the coefficients on the different loads are ~100, 1e-5 and 1e-5.

If we ask for a 100MW reduction, with the new code, we will scale by 2e-3MW at the first iteration and will exit the loop. Before, we would have scaled by 2e-3MW at the first iteration, renormalized the coefficients (since load1 is saturated), and then at the second iteration, scaled by 50MW on both loads 2 and 3.

indeed reverted the change causing this, was a bad idea, but maybe could be made different, e.g. if difference asked-done doesn't 'move' anymore by more than EPSILON between two iterations, then break the loop.

I am a bit worried of remaining very small values summed, summing many small floats can be surprising sometimes.

You opinion @phiedw ?

[phiedw]

I think this fix seems to be a decent way to fix the issue. I can't think of any cases where the behaviour would not be the expected one.
I was wondering if it could be possible to add to the scalable interface a method that checks some conditions to know if the scalable is saturated/disabled/disconnected etc to know if we should set its percentage to 0, but I'm not sure it would be cleaner.

[jeandemanged]

I think this fix seems to be a decent way to fix the issue. I can't think of any cases where the behaviour would not be the expected one. I was wondering if it could be possible to add to the scalable interface a method that checks some conditions to know if the scalable is saturated/disabled/disconnected etc to know if we should set its percentage to 0, but I'm not sure it would be cleaner.

yes I am not sure as well refactoring would make things easier

I was also wondering if to be on the safe side we should do as below, but cannot build a unit test for that.

saving it here so it doesn't get lost:

    private double iterativeScale(Network n, double asked, ScalingParameters parameters) {
        double done = 0;
        boolean first = false;
        while (Math.abs(asked - done) > EPSILON && notSaturated()) {
            checkIterationPercentages();
            double previousAskedMinusDone = asked - done;
            done += scaleIteration(n, asked - done, parameters);
            double newAskedMinusDone = asked - done;
            if (Math.abs(previousAskedMinusDone - newAskedMinusDone) < EPSILON) {
                if (!first) {
                    first = true;
                } else {
                    LOGGER.warn("Proportional iterative scale did not distribute more than {} MW between 2 consecutive iterations, " +
                            "stopping iterative scale. asked={} MW, done={} MW", EPSILON, asked, done);
                    break;
                }
            } else {
                first = false;
            }
            updateIterationPercentages();
        }
        return done;
    }