Avoid slack distribution to fictitious loads

[caioluke]

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)

What kind of change does this PR introduce?

Feature.

What is the current behavior?

Currently, LfLoads do not have the method isFictitious like LfGenerators do. Therefore, when distributing slack on loads, we do not take into account if its fictitious or not.

What is the new behavior (if this is a feature change)?
Avoid slack distribution to fictitious loads.

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

• Yes
• No

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[annetill]

Applying a contingency:

for (var e : lostLoads.entrySet()) {
    LfLoad load = e.getKey();
    LfLostLoad lostLoad = e.getValue();
    PowerShift shift = lostLoad.getPowerShift();
    load.setTargetP(load.getTargetP() - getUpdatedLoadP0(load, balanceType, shift.getActive(), shift.getVariableActive()));
    load.setTargetQ(load.getTargetQ() - shift.getReactive());
    load.setAbsVariableTargetP(load.getAbsVariableTargetP() - Math.abs(shift.getVariableActive()));
    lostLoad.getOriginalIds().forEach(loadId -> load.setOriginalLoadDisabled(loadId, true));
}

Here, the idea is to remove the effect of a load on a bus. If the load is fictitious, be careful:

• Is the shift ok for a fictitious load, knowing that the variable part is used for slack distribution.
• The updated load P0 that reflects the slack bus distribution on pre-contingency state must adapted?
• The abs variable targetP must be adapted?
  If you prefer, you can do a unit test on a LfNetwork and applying a contingency and check the values. See LfNetworkTest.

Same analysis must be done for actions.

[vidaldid-rte]

Did you remove the explicit import on purpose ?
Looks like many other files in this project don't use wilcards on import.
In case you use IntelliJ you may see https://sergiodelamo.com/blog/intellij-idea-disable-wildcard-imports.html for a configuration that does not put wildcards on import event with more than 3 classes imported from a package.

This is cleaner in cases where the same class name can be user in different packages.

[caioluke]

Changed it back to what it was
Indeed, IntelliJ automatically swapped multiple imports for a wildcard import

[vidaldid-rte]

Luke,
I stepped your test and it happens that when the contingency is processed a PowerShift is created with a Variable active power. This happens in PropagatedContingency.getLoadPowerShift.

I am not very familiar with load compensation, but it looks to me as a bug.The contingency on a fictive load should not add a shift of variable active power for a fictive load, in my understanding.

So it seems that we already have duplicated code and several places to compute the active power of an IIDM Load.
It think it would would be better if there was a single function that returned this value in the LF wordls, that would contain the logic on fictive in this unique place. And that NO other place of OpenLoadFlow computes the variable power of a load.

[caioluke]

You are right, there is a problem in my implementation.

The test now reflects it.

I think I have to add the isFictitious information in the LfLostLoad object...

And then handle each lost load accordingly.

What do you think about it?

[vidaldid-rte]

For me the ideal solution would be that the logic that computes the variable active power of a load is in only one place.
That means for example that if in the future, you need to add a parameter to take fictive into account or not, you would have only one function to modify.
The the logic is spread in different places, for actions, for contingnencies etc... there is a maintainaility issue and a risk that if in the future that logic has to be modified it is done only in one place.

[caioluke]

Hi @vidaldid-rte, I have just fixed the behaviour. We have something that is working now.

Could you have a look at this final version and tell me what you think?

I agree with you that the fictitious load information is all over the place (contingency, action), but I don't see another way of doing it...
I would gladly hear your suggestions about that.

[vidaldid-rte]

@caioluke as discussed I commited a refactoring proposal. Here are the ideas:

• Add the concept of a passive load (a load that does not participte to compensation)
• Only use this concept in the public API
• The use of fictive to determine if a load is passive is only in LfLoadImpl
• A single place to compute the variable active power (now static in LfLoadImpl) . Other clases that need this quantty now call the function instead of duplicating the code.
• Place factories of PowerShift for load side by side in PowerShift. There are differences (for actions the LoadDetail extension is ignored). But now the code is close and the differences are known and understood (or can later be harmonized).
• Removed the LfLoadFunction isFictitious. I replaced this by a test that variableActivePower is non zero. Should be equivalent semantically or there was a bug in LfLoad creation/modification.

Let me know if this looks correct to you.
The next step would then be to check the doc (doc directory) and update it to explain that fictive loads are ignored when the balance type is for loads.

[caioluke]

It looks much better!!! Thank you :)
I have only one comment: don't you think it's better to name isOriginalLoadPassive instead of isIidmLoadPassive? It is coherent with the other methods' naming, and it doesn't mention a data model

[vidaldid-rte]

No problem for the renaming. Go ahead !

[vidaldid-rte]

You should not keep directly iidm.network objects in memory since fast restart is implemented (#635)
If you need to keep the Load in a record, use a WeakRef instead.
Otherwise, this will prevent the GC from deleting a Network if the fast restart mode is on.

See LfBatteryImpl for an example.
Ref are created using com.powsybl.openloadflow.network.impl.Ref

[caioluke]

I see, thanks for the explanation
Since I don't need a lot of information about the load, only it's id and if it's fictitious or not, I kept only those informations instead of creating a Ref

[vidaldid-rte]

This implementation might be confusing to some users because it silently does something else than the user asked.
If the user created a LoadShift with a fictive load, why do you want not to honnor this ?

In case you really want to do this, it would bet better to generate and error when the LoadShift is created with a fictive load, or at least Warning in the log.

Also it the fact of ignoring fictive load for load compensation a feature desired by ALL users of load compensation ? If not this implementation that is not controlled by a parameter might be considered as a breaking change.

[caioluke]

The shift is applied for any type of load.

lfLoad.setTargetP(lfLoad.getTargetP() + shift.getActive());
lfLoad.setTargetQ(lfLoad.getTargetQ() + shift.getReactive());
lfLoad.setAbsVariableTargetP(lfLoad.getAbsVariableTargetP() + getUpdatedLoadAbsVariableTargetP(loadIsFictitious, shift));

The only difference is that the absVariableTargetP is not updated if the load is fictitious.
From what I understood, this absVariableTargetP is only used for slack distribution on loads.
Therefore, it is aligned with what I'm proposing.

Your point is still valid though: maybe I should also add a parameter to define if the user wants to distribute slack on fictitious loads or not.
I assumed that would always be the case, but maybe it is not...

What do you think about that?

[vidaldid-rte]

[Edited]
Thanks for the explanation. I did not get that the action was applied for the constant part of the load.
Maye you don't need to to something special for the setter of VariableTargetP since the getter is hardcoded to return 0 when load is fictive.
That would reduce code duplication and improve maintainability.

For the parameter, they key is to know if some users have valid use cases to compensate on fictive loads. I don't hace the answer. If nobody has a need, it's OK to make it a default BUT AT THE MINIMUM make the change visible in the relase notes If valid use cases to compensate on fictive loads exist then you have, unfortunately, to add a parameter to the 80 or more already existing... ANd to select a good default.

[caioluke]

I still need to do something special for the setter of variableTargetP..
In the LfLoad object, there may be only one load that is fictitious and not necessarily all of them.
The getter is hardcoded to return 0 only if all loads of a LfLoad are fictitious.
That was a tricky part of this PR.

I mentioned the need of an OLF parameter to @annetill and she thinks that it might be ok to pursue without one (she's not 100% sure).

[vidaldid-rte]

Thanks for the explanation. Talking of several loads on a bus there are other events that may change the list of loads during during security analysis (disconnect / disable events). Have you taken this into account ? I will try to review the code with this in mind and see if there is a way to centralize more this logic).

[caioluke]

I added this test, where I create a contingency for fictitious loads (only load on a bus and one fictitious load amongst many loads)

[vidaldid-rte]

Why do you need to do somthing special for fictive load in setAbsVariableTargetP for fictive loads since the getAbsVariableTargetP is harcoded for fictive load , by this new code::
private double getAbsVariableTargetP(Load load) {
if (isLoadFictitious(load)) {
return 0.0;
}

[caioluke]

Ha, you are right.
I didn't realize that.
Reverted back to what it was.

[annetill]

@vidaldid-rte I suggest to change to "updateNotParticipatingLoadP0"

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[caioluke]

Do we need to define the object as the implementation class? @vidaldid-rte

Map<String, Double> instead of HashMap<String, Double>

[vidaldid-rte]

You mean define it as a Map ? It avoids some pedantic discussions with Sonar lovers so why not, although it does not really matter in a private field definition.
There is still an issue with that - in theory it slows down iterations on the array (that happen frequently) because whe would break the processor cache here. I don't know how much it matters in real perf.
But we may revert this change and instead add commentarys that it is important to update the array and the map in sync because the code relies on the order.