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.

Sign up for GitHub

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

Jump to bottom

Cgmes export fix tap changer control issue #2716

Merged
merged 19 commits into from
Oct 20, 2023
Merged

Cgmes export fix tap changer control issue #2716

merged 19 commits into from
Oct 20, 2023

Conversation

marqueslanauja
Copy link
Contributor

@marqueslanauja marqueslanauja commented Sep 22, 2023 •
edited by zamarrenolm
Loading

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 one of the items of #2707 (TapChangerControl missing (phase control and voltage control))

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

The information required to export a tap changer control to CGMES is stored in the CgmesTapChanger extension in IIDM.
Starting from an IIDM Network not imported from CGMES, or making changes to the tap changer control after importing from CGMES, may result in the extension not being created or incomplete.

What is the new behavior (if this is a feature change)?
Before export, it is ensured that all required information about tap changer regulation exists in the CgmesTapChanger extension.

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

  • 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:

@marqueslanauja
Fix issue
4ac5f15 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
@marqueslanauja
add unitary test
fbbf0f3 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
@marqueslanauja marqueslanauja changed the title Cgmes export fix tap changer control issue 2707 Cgmes export fix tap changer control issue Sep 22, 2023
zamarrenolm
zamarrenolm reviewed Oct 6, 2023
View reviewed changes
cgmes/cgmes-conversion/src/main/java/com/powsybl/cgmes/conversion/export/CgmesExportUtil.java Outdated
@@ -308,6 +309,86 @@ public static <C extends Connectable<C>> void setCgmesTapChangerType(C eq, Strin
}
}

// tap changer is exported as it is modelled in IIDM, always at end 1
public static void addTapChangerExtensionsNecessaryForExporting(TwoWindingsTransformer twt) {
Copy link
Member

Choose a reason for hiding this comment

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

naming suggestion: use addUpdateCgmesTapChangerExtension instead of addTapChangerExtensionsNecessaryForExporting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@zamarrenolm zamarrenolm marked this pull request as ready for review October 6, 2023 08:42
marqueslanauja and others added 2 commits October 6, 2023 10:50
zamarrenolm
zamarrenolm reviewed Oct 6, 2023
View reviewed changes
cgmes/cgmes-conversion/src/main/java/com/powsybl/cgmes/conversion/export/CgmesExportUtil.java Outdated
}

private static boolean regulatingControlIsDefined(RatioTapChanger rtc) {
return !Double.isNaN(rtc.getTargetV()) && rtc.getTargetV() > 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 also check that regulating terminal is not null

Copy link
Member

Choose a reason for hiding this comment

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

and/or verify the flag for load tap changing capabilities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Only the regulating terminal checking is needed

@marqueslanauja
Add regulating terminal check
9ef15c8 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
@marqueslanauja
Improve control is defined
3ea65b1 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
@marqueslanauja
Verify that tapChangerControl is only exported when it is defined
18f39de 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
@marqueslanauja marqueslanauja marked this pull request as draft October 6, 2023 12:51
marqueslanauja and others added 2 commits October 6, 2023 16:44
@zamarrenolm zamarrenolm marked this pull request as ready for review October 9, 2023 06:42
@zamarrenolm zamarrenolm self-requested a review October 9, 2023 06:42
@nemanja-st
Copy link
Contributor

Merging TapChanger on PowerTransformerEnd might lead to loss of TapChanger and TapChangerControl in exported SSH and SV cimxml files.
This poses a risk that SSH and SV are unusable to user for power flow analysis because not all equipment can be mapped.

@zamarrenolm
Copy link
Member

Merging TapChanger on PowerTransformerEnd might lead to loss of TapChanger and TapChangerControl in exported SSH and SV cimxml files. This poses a risk that SSH and SV are unusable to user for power flow analysis because not all equipment can be mapped.

For power flow analysis we need only to export SSH and SV (no need to re-export the EQfile). In this situation, data about the "hidden" tap changer is still exported in the SSH, with the attribute controlEnabled set to false.

It is true that the tap changer control data is still not exported, but we have to consider that this will only happen if there are two tap changer controls in the transformer, one on each end. That is, the transformer is able to regulate changing positions in two tap changers. If there is only one tap changer control in the transformer, we keep it as the "main" tap changer, and the other tap changer (without control) will be the "hidden" one.

@annetill
Copy link
Member

Merging TapChanger on PowerTransformerEnd might lead to loss of TapChanger and TapChangerControl in exported SSH and SV cimxml files. This poses a risk that SSH and SV are unusable to user for power flow analysis because not all equipment can be mapped.

For power flow analysis we need only to export SSH and SV (no need to re-export the EQfile). In this situation, data about the "hidden" tap changer is still exported in the SSH, with the attribute controlEnabled set to false.

It is true that the tap changer control data is still not exported, but we have to consider that this will only happen if there are two tap changer controls in the transformer, one on each end. That is, the transformer is able to regulate changing positions in two tap changers. If there is only one tap changer control in the transformer, we keep it as the "main" tap changer, and the other tap changer (without control) will be the "hidden" one.

Import and export are consistent, and our power flow cannot handle this kind of rare configuration. So I agree to not support that.

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 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

89.8% 89.8% Coverage
0.0% 0.0% Duplication

@annetill annetill merged commit 94577c3 into main Oct 20, 2023
6 checks passed
@annetill annetill deleted the cgmes_export_fix_tapChangerControl_issue_2707 branch October 20, 2023 07:41
olperr1 pushed a commit that referenced this pull request Nov 13, 2023
@marqueslanauja @olperr1
Cgmes export: fix tap changer control issue (#2716)
4332f91 
Signed-off-by: José Antonio Marqués <marquesja@aia.es>
(cherry picked from commit 94577c3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zamarrenolm zamarrenolm zamarrenolm approved these changes

@annetill annetill annetill approved these changes

@nemanja-st nemanja-st Awaiting requested review from nemanja-st

Assignees
No one assigned
Labels
PR: ready-to-be-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marqueslanauja @nemanja-st @zamarrenolm @annetill