Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger #3130

bc-rte · 2024-09-04T08:52:55Z

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?

This PR introduces adder initialized by copying an existing component.

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)

the NetworkImpl::newLine method now has two parametric implementations with one parameter.
Users that used network.newLine(null) need to cast null network.newLine((String) null) in order to pass compilation

Other information:

This PR is inspired from previous work there.

flo-dup

The whole mechanism is ok, but the interfaces introduced to think it as a generic solution makes it obscure or too complex I think (and besides it's not completely covering the usecase I think).

iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/LineAdderImpl.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/AdderUser.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/CurrentLimitsNestedAdder.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/AdderUser.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/NestedAdder.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/CurrentLimitsPayload.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/Network.java

iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/CurrentLimitsNestedAdderImpl.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/LineAdder.java

iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/TemporaryLimitNestedAdderImpl.java

flo-dup

In this PR, we'd better stick with the basic but tedious cases of PhaseTapChanger/RatioTapChanger/ReactiveCapabilityCurve.

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/Network.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/BranchAdder.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/PhaseTapChangerHolder.java

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/RatioTapChangerHolder.java

.gitignore

…ity curve as template Signed-off-by: CHIQUET Benoit <[email protected]>

Signed-off-by: CHIQUET Benoit <[email protected]>

iidm/iidm-impl/src/test/java/com/powsybl/iidm/network/impl/tck/TapChangerHolderTest.java

iidm/iidm-tck/src/test/java/com/powsybl/iidm/network/tck/AbstractTapChangerHolderTest.java

…actTapChangerHolderTest.java Co-authored-by: Florian Dupuy <[email protected]> Signed-off-by: bc-rte <[email protected]>

Signed-off-by: CHIQUET Benoit <[email protected]>

sonarqubecloud · 2024-10-02T08:13:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

flo-dup · 2024-11-06T12:22:13Z

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/RatioTapChangerHolder.java

+                .setTargetDeadband(ratioTapChanger.getTargetDeadband());
+        for (int tapPosition = ratioTapChanger.getLowTapPosition(); tapPosition <= ratioTapChanger.getHighTapPosition(); tapPosition++) {
+            RatioTapChangerStep step = ratioTapChanger.getStep(tapPosition);
+            adder.beginStep()


wouldn't it be interesting to add a A beginStep(D step) in TapChangerAdder interface? It's probably not very useful, but in that way each level would have its copy method.

I would prefer to stick to the perimeter you defined previously.

If the need for copy-adder on steps solidifies, they could be added in a separate PR.

Even if you consider the code duplication in PhaseTapChangerHolder and RatioTapChangerHolder?

After looking back into it, the issue I have with it is that beginStep carries a state that would be unrelated to existing step: the order in which steps are created currently encodes the tap position. Being able to initialize beginStep by copying a step with a different tap position may be confusing for users.

A helper class could be used to mutualize the code, but it is unclear to me whether this would be better than the current situation.

sonarqubecloud · 2024-12-02T11:57:35Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

bc-rte force-pushed the add_builders_from_existing_objects branch 2 times, most recently from 2a7fd64 to cd5ce9d Compare September 4, 2024 09:21

bc-rte requested a review from geofjamg September 6, 2024 13:50

bc-rte force-pushed the add_builders_from_existing_objects branch from 9645e84 to 4bfb458 Compare September 6, 2024 14:06

bc-rte requested a review from flo-dup September 9, 2024 11:39

flo-dup requested changes Sep 11, 2024

View reviewed changes

bc-rte force-pushed the add_builders_from_existing_objects branch 6 times, most recently from 53da2eb to c903d96 Compare September 16, 2024 09:30

bc-rte changed the title ~~WIP: Add builders from existing objects~~ Add builders from existing objects Sep 16, 2024

bc-rte changed the title ~~Add builders from existing objects~~ WIP: Add builders from existing objects Sep 16, 2024

bc-rte force-pushed the add_builders_from_existing_objects branch from c903d96 to 347db8e Compare September 16, 2024 09:36

bc-rte changed the title ~~WIP: Add builders from existing objects~~ Add builders from existing objects Sep 16, 2024

bc-rte requested a review from flo-dup September 16, 2024 11:02

So-Fras added October 2024 release and removed October 2024 release labels Sep 18, 2024

flo-dup requested changes Sep 30, 2024

View reviewed changes

flo-dup changed the title ~~Add builders from existing objects~~ Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger Sep 30, 2024

bc-rte force-pushed the add_builders_from_existing_objects branch 2 times, most recently from 347db8e to 9687aae Compare September 30, 2024 12:25

bc-rte requested a review from flo-dup September 30, 2024 12:28

flo-dup reviewed Sep 30, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

bc-rte added 2 commits September 30, 2024 16:06

add reactive capability curve builder using existing reactive capabil…

171c4b5

…ity curve as template Signed-off-by: CHIQUET Benoit <[email protected]>

add way to build tap changers by copying existing ones

6180bf7

Signed-off-by: CHIQUET Benoit <[email protected]>

bc-rte force-pushed the add_builders_from_existing_objects branch from 9687aae to 6180bf7 Compare September 30, 2024 14:06

bc-rte requested a review from flo-dup September 30, 2024 14:06

flo-dup approved these changes Oct 1, 2024

View reviewed changes

Merge branch 'main' into add_builders_from_existing_objects

22000de

flo-dup requested changes Oct 1, 2024

View reviewed changes

iidm/iidm-impl/src/test/java/com/powsybl/iidm/network/impl/tck/TapChangerHolderTest.java Show resolved Hide resolved

iidm/iidm-tck/src/test/java/com/powsybl/iidm/network/tck/AbstractTapChangerHolderTest.java Outdated Show resolved Hide resolved

Update iidm/iidm-tck/src/test/java/com/powsybl/iidm/network/tck/Abstr…

629615e

…actTapChangerHolderTest.java Co-authored-by: Florian Dupuy <[email protected]> Signed-off-by: bc-rte <[email protected]>

bc-rte requested a review from flo-dup October 2, 2024 07:47

license

a239827

Signed-off-by: CHIQUET Benoit <[email protected]>

bc-rte force-pushed the add_builders_from_existing_objects branch from 95d0bca to a239827 Compare October 2, 2024 08:03

bc-rte added the PR: waiting-for-review label Oct 29, 2024

flo-dup reviewed Nov 6, 2024

View reviewed changes

flo-dup approved these changes Dec 2, 2024

View reviewed changes

Merge branch 'main' into add_builders_from_existing_objects

93eb8d8

flo-dup added December 2024 release and removed PR: waiting-for-review labels Dec 2, 2024

Merge branch 'main' into add_builders_from_existing_objects

6919780

flo-dup merged commit 94abfaa into main Dec 2, 2024
7 checks passed

flo-dup deleted the add_builders_from_existing_objects branch December 2, 2024 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger #3130

Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger #3130

bc-rte commented Sep 4, 2024 •

edited

Loading

flo-dup left a comment

flo-dup left a comment

sonarqubecloud bot commented Oct 2, 2024

flo-dup Nov 6, 2024

bc-rte Nov 6, 2024

flo-dup Nov 6, 2024

bc-rte Nov 25, 2024

sonarqubecloud bot commented Dec 2, 2024

Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger #3130

Create adders from existing objects: ReactiveCapabilityCurve, PhaseTapChanger, RatioTapChanger #3130

Conversation

bc-rte commented Sep 4, 2024 • edited Loading

flo-dup left a comment

Choose a reason for hiding this comment

flo-dup left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 2, 2024

Quality Gate passed

flo-dup Nov 6, 2024

Choose a reason for hiding this comment

bc-rte Nov 6, 2024

Choose a reason for hiding this comment

flo-dup Nov 6, 2024

Choose a reason for hiding this comment

bc-rte Nov 25, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 2, 2024

Quality Gate passed

bc-rte commented Sep 4, 2024 •

edited

Loading