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

CGMES import: active power control extension is optional #2462

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

annetill
Copy link
Member

@annetill annetill commented Feb 2, 2023

Signed-off-by: Anne Tilloy anne.tilloy@rte-france.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)

  • 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 ? If so, link to this issue using '#XXX' and skip the rest

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

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

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@annetill annetill requested review from zamarrenolm and miovd February 2, 2023 07:58
.add();
if (context.config().createActivePowerControlExtension()) {
g.newExtension(ActivePowerControlAdder.class)
.withParticipate(normalPF != 0.0)
Copy link
Member

Choose a reason for hiding this comment

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

we should probably change this to .withParticipate(true) always

CGMES does not have an equivalent for this participate attribute

then load flow implementations can use distribution proportional to Pmax, to targetP, to remaining margin, independently of this participate flag for iIDMs created from CGMES

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, I was thinking about that too... One of our test is failing because only one generator has a participating to true.

Copy link
Member

Choose a reason for hiding this comment

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

Formal documentation about CGMES participation factor (normalPF) only says it is used for distributed slack: Generating unit economic participation factor. The sum of the participation factors across generating units does not have to sum to one. It is used for representing distributed slack participation factor. The attribute shall be a positive value or zero.

It seems correct to set the flag participating to false when normalPF is zero, but if that introduces noise, I agree on setting it always to true, and let the Load Flow engines decide later if the unit is effectively used for distributed slack looking the participationFactor value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @zamarrenolm for these clarifications and your approval :-)

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@@ -298,6 +304,7 @@ private void copyStream(ReadOnlyDataSource from, DataSource to, String fromName,
public static final String SOURCE_FOR_IIDM_ID = "iidm.import.cgmes.source-for-iidm-id";
public static final String STORE_CGMES_MODEL_AS_NETWORK_EXTENSION = "iidm.import.cgmes.store-cgmes-model-as-network-extension";
public static final String STORE_CGMES_CONVERSION_CONTEXT_AS_NETWORK_EXTENSION = "iidm.import.cgmes.store-cgmes-conversion-context-as-network-extension";
public static final String CREATE_ACTIVE_POWER_CONTROL_EXTENSION = "iidm.import.cgmes.create-active-power-extension";
Copy link
Member

Choose a reason for hiding this comment

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

iidm.import.cgmes.create-active-power-control-extension

@@ -810,6 +810,15 @@ public Config setStoreCgmesConversionContextAsNetworkExtension(boolean storeCgme
return this;
}

public boolean createActivePowerControlExtension() {
return createActivePowerExtension;
Copy link
Member

Choose a reason for hiding this comment

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

createActivePowerControlExtension

return createActivePowerExtension;
}

public Config setCreateActivePowerExtension(boolean createActivePowerExtension) {
Copy link
Member

Choose a reason for hiding this comment

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

setCreateActivePowerControlExtension

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Luma <zamarrenolm@aia.es>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

87.5% 87.5% Coverage
0.0% 0.0% Duplication

@miovd miovd merged commit 240ab01 into main Feb 2, 2023
@miovd miovd deleted the active-power-extension-optional branch February 2, 2023 11:00
miovd pushed a commit that referenced this pull request Feb 2, 2023
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Co-authored-by: Luma <zamarrenolm@aia.es>
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.

5 participants