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] Automatic fix of incompatible target voltages #1115

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

Conversation

geofjamg
Copy link
Member

@geofjamg geofjamg commented Nov 6, 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?

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior (if this is a feature change)?

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

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

geofjamg and others added 14 commits November 4, 2024 20:34
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
@@ -276,6 +276,8 @@ public enum FictitiousGeneratorVoltageControlCheckMode {

public static final String AREA_INTERCHANGE_P_MAX_MISMATCH_PARAM_NAME = "areaInterchangePMaxMismatch";

public static final String FIX_TARGET_VOLTAGE_INCOMPATIBILITY_PARAM_NAME = "fixTargetVoltageIncompatibility";

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be documented.
And maybe addiotional parameters given like TARGET_VOLTAGE_PLAUSIBILITY_THRESHOLD

However in my test networks, increasing CONTROLLED_BUS_NEIGHBORS_EXPLORATION_DEPTH had a terrible algotirhmic cost and didn't improve quality (defined as more convergence)

Copy link
Member Author

Choose a reason for hiding this comment

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

"And maybe addiotional parameters given like TARGET_VOLTAGE_PLAUSIBILITY_THRESHOLD" => yes
for CONTROLLED_BUS_NEIGHBORS_EXPLORATION_DEPTH indeed it should not be configurable, a value of 2 or 3 is probably enough.

PiModel piModel = branch.getPiModel();
if (FastMath.abs(piModel.getX()) < LfNetworkParameters.LOW_IMPEDANCE_THRESHOLD_DEFAULT_VALUE) {
if (bus1 != null && bus2 != null) {
LOGGER.warn("Non impedant branches ({}) not supported in admittance matrix",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the impact of ignoring those branches ? Could this lead to failure to detect inconsistent voltage target in nearby stations connected by short lines ?
Wouldn't it be better to merge the buses in the graph ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, I cut the impedance using the LfNetworkParameters.LOW_IMPEDANCE_THRESHOLD_DEFAULT_VALUE which is ok for what we are doing with admittance matrix.

public static AdmittanceMatrix create(AdmittanceEquationSystem ySystem, MatrixFactory matrixFactory) {
Objects.requireNonNull(matrixFactory);
try (var j = new JacobianMatrix<>(ySystem.getEquationSystem(), matrixFactory)) {
return new AdmittanceMatrix(ySystem.getEquationSystem(), j.getMatrix());
Copy link
Collaborator

@vidaldid-rte vidaldid-rte Nov 15, 2024

Choose a reason for hiding this comment

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

Wouldn't it be cleaner to inherit from JacobianMatrix ?
Here if a future evolution of JacobianMatrix closes the Matrix object the code will break because it assumes today that the matrix is still alive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Admittance matrix has nothing to do with Jacobian matrix even if we use it to compute admittance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well - you reused the matrix factory part. And steal the matrix of a closable - in addition a matrix with a DirectBuffer in it. That works today, and in your code structure but is not really a reusable object. Not a big deal and not reason to disapprove. But I can't stay silent when I see this :-)

var i1yEq = equationSystem.getEquation(bus1.getNum(), AdmittanceEquationType.BUS_ADM_IY).orElseThrow();
var i2xEq = equationSystem.getEquation(bus2.getNum(), AdmittanceEquationType.BUS_ADM_IX).orElseThrow();
var i2yEq = equationSystem.getEquation(bus2.getNum(), AdmittanceEquationType.BUS_ADM_IY).orElseThrow();
if (e == null) {
Copy link
Collaborator

@vidaldid-rte vidaldid-rte Nov 15, 2024

Choose a reason for hiding this comment

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

This is definitely not MT safe. Not only that it could lead to double allocation. But because the matrix is reused.
Can you document the limitation in the class ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed but I don't think it is expected to be thread safe as all of other similar OLF linear algebra utilities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I didn-t know the underlyings I would expect it to be threadsafe. Maybe a general problem to OLF.
But running it in MT mode requires a lot of expertis today (see previous PR an MT security analysis).
Anyway, out of scope of ths feature.


var ySystem = AdmittanceEquationSystem.create(network, new VariableSet<>());
try (var y = AdmittanceMatrix.create(ySystem, parameters.getMatrixFactory())) {
for (LfBus controlledBus : controlledBuses) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you compute the impedance between each pair of bus twice ? if you iterate on all buses then on all neighbours ?

Also, would it accerlerate things if there was an option to check a pair only if at least one of the two buses is remotely controlled ? [This is where we have convergence issues]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we are checking potentially twice. It could be improved indeed.

Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Signed-off-by: Geoffroy Jamgotchian <[email protected]>
Copy link

sonarcloud bot commented Nov 17, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
46.9% Coverage on New Code (required ≥ 90%)

See analysis details on SonarQube Cloud

@geofjamg
Copy link
Member Author

@vidaldid-rte @SylvestreSakti I added in addition to "incompatible target v" check a new "unrealistic target v" which rely on target voltage to calculate voltage sensitivities (already existing in OLF) to estimate to controller bus voltage given a controlled bus targe voltage and discard when too far from 1pu. On my real test case it works very well.

return findElementsToDiscardFromVoltageControl(network, parameters, new SparseMatrixFactory());
}

public static Set<String> findElementsToDiscardFromVoltageControl(Network network, LoadFlowParameters parameters, MatrixFactory matrixFactory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a reason ?
Incompatible with XXX
Irrealistic

(It seems at this time there is even no log to understand the discard recommendation)

return (EquationTerm<AcEquationType, AcEquationType>) controllerBus.getCalculatedV();
}

private void checkUnrealisticTargets(RemoteVoltageTargetCheckerParameters parameters,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good idea !

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.

2 participants