Skip to content
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

[WIP] Area Interchange Control as OuterLoop #1055

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

m-guibert
Copy link

@m-guibert m-guibert commented Jul 1, 2024

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?

Fixes #978

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

  • Yes
  • No

@m-guibert m-guibert self-assigned this Jul 1, 2024
@m-guibert m-guibert force-pushed the area_interchange_control_outerloop branch from 91829f9 to 7bbd807 Compare July 1, 2024 16:02
Signed-off-by: mguibert <marine.guibert@artelys.com>
@m-guibert m-guibert force-pushed the area_interchange_control_outerloop branch from 7bbd807 to 08e2260 Compare July 3, 2024 14:39
…e Control

Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
@@ -618,6 +630,24 @@ public boolean isVoltagePerReactivePowerControl() {
return voltagePerReactivePowerControl;
}

public boolean isAreaInterchangeControl() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move all these new methods above isVoltagePerReactivePowerControl, as isVoltagePerReactivePowerControl and setVoltagePerReactivePowerControl should remain next to each other

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutliple commits changed this, last one is 42bfe28 and this is fixed now

@@ -1639,6 +1677,7 @@ static LfNetworkParameters getNetworkParameters(LoadFlowParameters parameters, O
.setUseLoadModel(parametersExt.isUseLoadModel())
.setSimulateAutomationSystems(parametersExt.isSimulateAutomationSystems())
.setReferenceBusSelector(ReferenceBusSelector.fromMode(parametersExt.getReferenceBusSelectionMode()))
.setAreaInterchangeControlAreaType(parametersExt.getAreaInterchangeControlAreaType())
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the line below above this one:
.setAreaInterchangeControl(parametersExt.isAreaInterchangeControl())

Also, both new lines should probably be mmoved just above the setTransformerVoltageControl call? (Everywhere else, the AIC new parameters/getter/setters are added next to it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmouradian
cc @m-guibert

current practice is to put new parameters at the bottom

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in 743827b

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in 743827b

I see you moved the line to follow Damien's instructions but you didn't add the line I asked for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adressed in 1b27134, and cleaned in a7c6a58 (Damien <3)

import com.powsybl.openloadflow.network.BoundaryFactory;
import org.junit.jupiter.api.Test;

class AICTests {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to AreaInterchangeControlTest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change in 63eb397

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost: you wrote TestS instead of Test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 fixed in e83f8af

Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to LfArea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed in 743827b

Comment on lines 436 to 437
updateControlAreaBoundaryP(branch.getTerminal1(), loadingContext, lfBranch::getP1);
updateControlAreaBoundaryP(branch.getTerminal2(), loadingContext, lfBranch::getP2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update is not the right term, add instead ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this, mostly in 097d801 and f3930a8

Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
@vmouradian vmouradian self-assigned this Jul 18, 2024
return this;
}

public OpenLoadFlowParameters setVoltagePerReactivePowerControl(boolean voltagePerReactivePowerControl) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmouradian it looks like you moved this function by error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 oups. Changed in 5a29ee9

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is still a problem: you didn't move it back to its initial place. See the diff view to check if you rolled-back correctly all those modifications
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not remember the last commit on this but the diff seems ok now

@vmouradian vmouradian force-pushed the area_interchange_control_outerloop branch 2 times, most recently from c4db184 to 26da6ec Compare July 19, 2024 12:25
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
@vmouradian vmouradian force-pushed the area_interchange_control_outerloop branch from 26da6ec to 5a29ee9 Compare July 19, 2024 14:28
vmouradian and others added 10 commits July 25, 2024 10:15
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: Damien Jeandemange <damien.jeandemange@artelys.com>
Signed-off-by: Damien Jeandemange <damien.jeandemange@artelys.com>
@@ -19,13 +21,27 @@
*/
abstract class AbstractAcOuterLoopConfig implements AcOuterLoopConfig {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExplicitAcOuterLoopConfig needs also to be modified to include new AreaInterchangeControl OL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this one unchanged, and make another one extending it specifically for AIC, containing also the details of distributed power per area

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeandemanged I agree, and this makes me think that we should change AcLoadFlowResult to have distributed active power per area. Is that also what you were thinking about ?

Comment on lines 28 to 30
void addBus(LfBus bus);

void addBoundaryP(Supplier<Evaluable> p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addBus and addBoundaryP should not be part of LfArea interface, this is only internal to LfNetworkLoaderImpl

Comment on lines 32 to 42
double getSlackInjection(double slackBusActivePowerMismatch);

void addExternalBusSlackParticipationFactor(LfBus bus, double shareFactor);

/**
* Some buses are not in the Area, but their slack should be considered for the slack of the Area
* (This happens for example when a bus is connected to multiple Areas but the flow through the bus is not considered for those areas' interchange power flow).
* The slack of this buses can need to be shared with other Areas with a factor for each area.
* @return A map of the external buses considered for the slack of the Area with the share factor of the Area
*/
Map<LfBus, Double> getExternalBusesSlackParticipationFactors();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be part of LfArea interface, this is internal to AIC outerloop


private Map<LfBus, Double> externalBusesSlackParticipationFactors = new HashMap<>();

public LfAreaImpl(Area area, LfNetwork network, LfNetworkParameters parameters) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected

Comment on lines 74 to 82
@Override
public void addBus(LfBus bus) {
buses.add(bus);
}

@Override
public void addBoundaryP(Supplier<Evaluable> getP) {
boundariesP.add(getP);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buses and boundaries should be part of constructor/create

Comment on lines 84 to 102
@Override
public double getSlackInjection(double slackBusActivePowerMismatch) {
int totalSlackBusCount = (int) getNetwork().getSlackBuses().stream().count();
int areaSlackBusCount = (int) getBuses().stream().filter(LfBus::isSlack).count();
double areaExternalSlackBusShare = getExternalBusesSlackParticipationFactors().entrySet().stream().filter(entry -> entry.getKey().isSlack()).mapToDouble(Map.Entry::getValue).sum();
return totalSlackBusCount == 0 ? 0.0 : slackBusActivePowerMismatch * (areaSlackBusCount + areaExternalSlackBusShare) / totalSlackBusCount;
}

@Override
public void addExternalBusSlackParticipationFactor(LfBus bus, double shareFactor) {
if (bus != null) {
externalBusesSlackParticipationFactors.put(bus, shareFactor);
}
}

@Override
public Map<LfBus, Double> getExternalBusesSlackParticipationFactors() {
return externalBusesSlackParticipationFactors;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic should not be here, but in the AIC outerloop

Copy link
Member

@jeandemanged jeandemanged Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be revised so that LfArea-s are created once with all buses and boundaries without being mutated further, there should not be any addBus or addBoundary method in LfArea or LfAreaImpl

also need to include LfArea to LfNetworkLoaderPostProcessor with a new method

void onAreaAdded(Object element, LfArea lfArea);

Comment on lines 987 to 990
List<LfBus> handledBuses = lfNetwork.getAreas().stream().flatMap(area ->
area.getBuses().stream())
.toList();
List<LfBus> unhandledBuses = lfNetwork.getBuses().stream().filter(b -> !handledBuses.contains(b)).toList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance issue here, especially the .filter(b -> !handledBuses.contains(b)) because handledBuses is a List.

changing to a Set could be an option, but we can see here that we need in LfBus a method like

Optional<LfArea> getLfArea()

Comment on lines 1007 to 1014
// If the flow through this bus is considered for the interchange of some of the connected areas,
// then it has already been added to the Area's externalBusesSlackParticipationFactors map with a factor 0.0
// Here we want to share the slack injection of this bus between the areas that are not considering the flow through this bus for their interchange power flow
Set<LfArea> areasSharingSlack = connectedAreas.stream().filter(area -> !area.getExternalBusesSlackParticipationFactors().containsKey(unhandledBus)).collect(Collectors.toSet());
if (!areasSharingSlack.isEmpty()) {
areasSharingSlack.forEach(area -> area.addExternalBusSlackParticipationFactor(unhandledBus, 1.0 / areasSharingSlack.size()));
LOGGER.info("Bus {} is not in any Area and is connected to Areas: {} but the flow through this bus is not considered for their interchange flow. For a correct balancing, its slack injection will be shared between those areas", unhandledBus.getId(), connectedAreas.stream().map(LfArea::getId).toList());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this, and probably the entire method, should be moved outside LfNetworkLoader to be AIC outerloop code (e.g. in init part)

Comment on lines +1173 to +1184
static void checkDuplicatedAreas(List<LfNetwork> lfNetworks) {
List<String> duplicatedAreas = lfNetworks.stream()
.flatMap(lfNetwork -> lfNetwork.getAreas().stream())
.collect(Collectors.groupingBy(LfArea::getId, Collectors.counting()))
.entrySet().stream()
.filter(e -> e.getValue() > 1)
.map(Map.Entry::getKey)
.toList();
if (!duplicatedAreas.isEmpty()) {
throw new PowsyblException("Areas with ids " + duplicatedAreas + " are present in more than one LfNetwork. Load flow computation with area interchange control is not supported in this case.");
}
}
Copy link
Member

@jeandemanged jeandemanged Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this, in practice, you are going to throw for all real case networks which always have small portions here and there.

The correct check is verifying that each area has all its boundaries are in the same LfNetwork.

Also, should not throw, just warn

double distributedActivePower = areaActivePowerMismatch - remainingMismatch;
totalDistributedActivePower += distributedActivePower;
movedBuses |= result.movedBuses();
totalIterations += result.iteration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don' think this sum has anything meaningful, only the value per area is meaningful.

public interface LfArea extends PropertyBag {
String getId();

double getInterchangeTarget();
Copy link
Member

@jeandemanged jeandemanged Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please prepare also a setter, would be used later on when we will implement area actions

vmouradian and others added 2 commits August 2, 2024 10:20
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Co-authored-by: jeandemanged <damien.jeandemange@artelys.com>
Signed-off-by: Valentin Mouradian <144696437+vmouradian@users.noreply.github.com>
Copy link

sonarcloud bot commented Aug 2, 2024

Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
…l outerloop

Signed-off-by: vmouradian <valentin.mouradian@artelys.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Area Interchange Control as OuterLoop
3 participants