-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distributed slack management in sensitivity analysis #178
Conversation
@@ -30,11 +30,11 @@ | |||
*/ | |||
private static final double SLACK_P_RESIDUE_EPS = Math.pow(10, -5); | |||
|
|||
protected static class ParticipatingElement<T> { | |||
public static class ParticipatingElement<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really want to get rid of these "public" changes, but I need to get those values somewhere else in the code
Should I just create a protected method that returns something understandable by the rest of the code ?
(returning Pair<String, Number> instead of participatingElement for example)
src/main/java/com/powsybl/openloadflow/dc/equations/AbstractClosedBranchDcFlowEquationTerm.java
Outdated
Show resolved
Hide resolved
src/main/java/com/powsybl/openloadflow/sensi/DcSensitivityAnalysis.java
Outdated
Show resolved
Hide resolved
@@ -131,6 +145,38 @@ void testDc4buses() { | |||
assertEquals(0.25d, getValue(result, "g4", "l13"), LoadFlowAssert.DELTA_POWER); | |||
} | |||
|
|||
@Test | |||
void testDc4busesDistributed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test does not pass at the moment, it's [WIP]
if (loadFlowParameters.isDistributedSlack()) { | ||
switch (loadFlowParameters.getBalanceType()) { | ||
case PROPORTIONAL_TO_GENERATION_P_MAX: | ||
DistributedSlackOnGenerationOuterLoop outerLoop = new DistributedSlackOnGenerationOuterLoop(openLoadFlowParameters.isThrowsExceptionInCaseOfSlackDistributionFailure()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is were I need all this outer loop stuff to be Public, I would love to avoid this
* @param oldFactors | ||
* @return | ||
*/ | ||
private List<SensitivityFactor> getDistributedFactors(Network network, LfNetwork lfNetwork, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method uses network to get generators. But lfNetwork may not contain all these generators (because it is a connected component of network), so I would love to use only lfNetwork, but I do not know how to access generators from LfNetwork
* @param sensitivityValues | ||
* @param remainder | ||
* @return | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardly understandable, needs refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it is the part of the code that is the less understable for me.
List<SensitivityValue> sensitivityValues = calculateSensitivityValues(lfNetwork, equationSystem, factorsByVarConfig, states); | ||
|
||
if (!lfParameters.isDistributedSlack()) { | ||
return sensitivityValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a return in the middle of the function makes it harder to understand, I need to find a better way to distinguish the distributed/ not distributed case
76dc05f
to
71ee251
Compare
} | ||
|
||
switch (balanceType) { | ||
case PROPORTIONAL_TO_GENERATION_P_MAX: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Djazouli Geoffory prefers that we do not use factors that whould remain in the API. So we have to fix that.
* @param loadFlowParameters | ||
* @return | ||
*/ | ||
private List<Pair<String, Number>> getParticipatingElements(LfNetwork lfNetwork, LoadFlowParameters loadFlowParameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we just have to have as argument the balance type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests have to be added. I think that this PR should not have more features but should be tested and improved before review.
} | ||
|
||
// compute sensitivities corrected from the slack distribution effects | ||
for (SensitivityValue sensitivityValue : sensitivityValues) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here a fix is needed because I don't have sort the sensitivity values to give only those that have been ask in the API. @Djazouli your code was functionaly right but it was to remove the use of factors.
642b814
to
db45230
Compare
assertEquals(0.275d, getValue(result, "g1", "l12"), LoadFlowAssert.DELTA_POWER); | ||
assertEquals(-0.125d, getValue(result, "g1", "l23"), LoadFlowAssert.DELTA_POWER); | ||
assertEquals(0.025d, getValue(result, "g1", "l34"), LoadFlowAssert.DELTA_POWER); | ||
assertEquals(0.15d, getValue(result, "g1", "l13"), LoadFlowAssert.DELTA_POWER); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo:
add tests:
- where we are injecting on a load node that has no generator
- Where the slack bus is participating in the compensation
- Where the balance type is PROPORTIONAL_TO_LOAD
We still need to test balance type : PROPORTIONAL_TO_LOAD to improve coverage |
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
…rs required for distribution Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Improve variable naming. Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
6e3b924
to
60cf4da
Compare
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some slight variable names have to be changed to improve readability. I have a question about injection increase for a generator or a load that is connected to the slack bus: for the moment, we throw an exception. Is it the final behaviour that we aim ?
Map<SensitivityVariableConfiguration, SensitivityFactorGroup> factorsByVarConfig = new LinkedHashMap<>(factors.size()); | ||
Map<String, Number> participationMap = getParticipationMap(network, lfNetwork, loadFlowParameters); // empty if slack is not distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe participationFactorMap ?
Map<SensitivityVariableConfiguration, SensitivityFactorGroup> factorsByVarConfig = new LinkedHashMap<>(factors.size()); | ||
Map<String, Number> participationMap = getParticipationMap(network, lfNetwork, loadFlowParameters); // empty if slack is not distributed | ||
participationMap.remove(lfNetwork.getSlackBus().getId()); // the injection on the slack bus will not appear in the rhs | ||
participationMap.replaceAll((key, value) -> -value.doubleValue()); // the compensation on a bus will be the opposite of its participation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compensation -> slack distribution
SensitivityVariableConfiguration varConfig = new SensitivityVariableConfiguration(Collections.singleton(bus.getId()), Collections.emptySet()); | ||
Map<String, Number> busCompensation = new HashMap<>(participationMap); | ||
// add 1 where we are making the injection | ||
busCompensation.put(bus.getId(), busCompensation.getOrDefault(bus.getId(), 0).doubleValue() + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
busCompensation is not the good name choice indeed.
} else if (participatingElement.getElement() instanceof LfBus) { | ||
busId = ((LfBus) participatingElement.getElement()).getId(); | ||
} else { | ||
throw new UnsupportedOperationException("Only buses can have an impact in slack distribution"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I am not sure that you can cover easily this line.
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
@@ -142,7 +142,7 @@ private DenseMatrix initRhs(LfNetwork lfNetwork, EquationSystem equationSystem, | |||
for (Map.Entry<String, Number> busAndInjection : configuration.busInjectionById().entrySet()) { | |||
LfBus lfBus = lfNetwork.getBusById(busAndInjection.getKey()); | |||
if (lfBus.isSlack()) { | |||
throw new PowsyblException("Cannot analyse sensitivity of slack bus"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to get why this line is not cover by our test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because we never reach this line, the slack bus will never be inside the businjectionbyId. But the exception here stays for the safety of future developments
src/main/java/com/powsybl/openloadflow/network/impl/LfBranchImpl.java
Outdated
Show resolved
Hide resolved
participatingElements = step.getParticipatingElements(lfNetwork); | ||
ParticipatingElement.normalizeParticipationFactors(participatingElements, "bus"); | ||
|
||
participatingElements.forEach(participatingElement -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly create the map like:
Map<String, Number> participationFactorByBusMap = participatingElements.stream().collect(Collectors.toMap(..));
// run DC load | ||
Map<String, Double> functionReferenceByBranch = new HashMap<>(); | ||
DcLoadFlowParameters dcLoadFlowParameters = new DcLoadFlowParameters(lfParametersExt.getSlackBusSelector(), | ||
new SparseMatrixFactory(), true, lfParametersExt.isDcUseTransformerRatio(), lfParameters.isDistributedSlack(), lfParameters.getBalanceType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatrixFactory should be taken from parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but tp be checked by @Djazouli !
* @param loadFlowParameters | ||
* @return | ||
*/ | ||
private Map<String, Number> getParticipationFactorByBus(Network network, LfNetwork lfNetwork, LoadFlowParameters loadFlowParameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map<String, Double> ?
@@ -36,17 +39,17 @@ | |||
|
|||
static class SensitivityVariableConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should remove SensitivityVariableConfiguration? I added it to avoid computing same sensibility multiple times but there is no reason for that to happen frequently (user input mistake?) and it adds useless complexity.
So instead of Map<SensitivityVariableConfiguration, SensitivityFactorGroup> factorsByVarConfig
we would only have List<SensitivityFactor<?, ?>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like SensitivityVariableConfigurations are useful when the user gives a provider that contains all the generators of the network. Not sure if this is considered as an "user input mistake". But in this case, the SensitivityVariableConfiguration is useful to remove duplicate (generators connected on the same bus).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so let s keep it
Map<String, Double> functionReferenceByBranch = new HashMap<>(); | ||
DcLoadFlowParameters dcLoadFlowParameters = new DcLoadFlowParameters(lfParametersExt.getSlackBusSelector(), | ||
new SparseMatrixFactory(), true, lfParametersExt.isDcUseTransformerRatio(), lfParameters.isDistributedSlack(), lfParameters.getBalanceType()); | ||
DcLoadFlowResult dcLoadFlowResult = new DcLoadFlowEngine(lfNetworks, dcLoadFlowParameters).run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we calculate sensitivity only on first component do we really need to run the initial DC loadflow on all components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, in the DCLoadFlowEngine, we explicitely compute the LF just on the first connected component. So I leave it like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
participatingElements.forEach(participatingElement -> { | ||
String busId; | ||
if (participatingElement.getElement() instanceof LfGenerator) { | ||
busId = network.getGenerator(((LfGenerator) participatingElement.getElement()).getId()).getTerminal().getBusView().getBus().getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid searching generator into IIDM model. We could just get LfBus from LfGenerator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed there is no existing method, you mean that we have to implement the getter on all LfGenerator or there is something easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried something in the latest commit, tell me if this works for you:
Map<String, Double> functionReferenceByBranch = new HashMap<>(); | ||
DcLoadFlowParameters dcLoadFlowParameters = new DcLoadFlowParameters(lfParametersExt.getSlackBusSelector(), | ||
new SparseMatrixFactory(), true, lfParametersExt.isDcUseTransformerRatio(), lfParameters.isDistributedSlack(), lfParameters.getBalanceType()); | ||
DcLoadFlowResult dcLoadFlowResult = new DcLoadFlowEngine(lfNetworks, dcLoadFlowParameters).run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for later but running a DC LF using DcLoadFlowEngine is not efficient as we later need to build the same equation system and jacobian for sensitivity analysis.
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
… factor map Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @Djazouli and @Hadrien-Godard for this great PR !
Signed-off-by: Gael Macherel gael.macherel@artelys.com
Please check if the PR fulfills these requirements (please use
'[x]'
to check the checkboxes, or submit the PR and then click the checkboxes)What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Create a feature allowing the user to use distributed slack in sensitivity analysis
Does this PR introduce a breaking change or deprecate an API? If yes, check the following:
Other information:
This is currently WIP, I opened the PR to request feedback