Introduction of non linear shunt compensators #1202

miovd · 2020-03-04T08:03:33Z

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 ?
fix #900
fix #87

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

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:
This PR only deals with the change of API. XIIDM serialization and deserialization will be done in another PR.

mathbagu

I will redo a complete review soon

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

...cripting/src/main/groovy/com/powsybl/iidm/network/scripting/ShuntCompensatorExtension.groovy

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

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

ampl-converter/src/main/java/com/powsybl/ampl/converter/AmplNetworkWriter.java

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

mathbagu · 2020-03-06T10:02:28Z

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


-    ShuntCompensatorAdder setMaximumSectionCount(int maximumSectionCount);
+    ShuntCompensatorNonLinearModelAdder newNonLinearModel();


To be discussed: should we design the API like we did for the extension:

newModel(ShuntCompensatorLinearModelAdder.class);

Do you think that choosing the same design here could improve the user experience?

It is mostly to keep an easy way to add a custom model... but doesn't it make the API more complex? (as in, it may make it more difficult to understand what "model" refers to)? Maybe we should keep newLinearModel, newNonLinearModel and add newModel(...)?

EDIT I am not sure that the denomination "shunt model" is standard. In CGMES for example, "shunt models" are not mentioned, the concepts are directly "non linear shunt" and "linear shunt".

After some thinking, I do not think it is a good idea to implement a newModel(...) method, I am not sure we want to extend the possibilities of shunt models. It is different from extensions where this is the purpose of the API.

mathbagu · 2020-03-06T10:05:38Z

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

+    @Override
+    public void setShuntCompensator(ShuntCompensatorImpl shuntCompensator) {
+        this.shuntCompensator = Objects.requireNonNull(shuntCompensator);
+    }


I'm not sure about this: the model should be attached to the ShuntCompensator in the constructor, like it's done for tap changer

It doesn't work as tapchangers are created when the transformer is already built, which is not the case for models (they are created at the same time since it does not make sense to create a shunt without model).

I can check however that the shunt is only defined once. Is it enough for you?

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

mathbagu · 2020-03-06T10:08:32Z

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

+                ValidationUtil.checkSectionNumber(ShuntCompensatorAdderImpl.this, sectionNum);
+                if (sections.containsKey(sectionNum)) {
+                    throw new ValidationException(ShuntCompensatorAdderImpl.this, "a section is already defined at this number");
+                }


It's strange to not replace the section whereas it's totally allowed later? It's not really important but it's questioning me

Not sure if choosing to replace a section in a built model and creating the same section twice while building the whole model is the same thing? I think the second case is more probably an error but I may be mistaken. We can delete this validation condition and consider it is allowed to rewrite a section while building the model.

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

mathbagu · 2020-03-06T10:19:17Z

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

+
+    <M extends ShuntCompensatorModel> M getModel(Class<M> modelType);
+
+    void setShuntCompensator(ShuntCompensatorImpl shuntCompensator);


Not sure this method should be available

See comment above

mathbagu

Could you please check again if everything in the merging view is implemented? I think we should have adapter for the model since you have a backward link. Is it required to have this link?

mathbagu · 2020-03-13T14:32:41Z

...cripting/src/main/groovy/com/powsybl/iidm/network/scripting/ShuntCompensatorExtension.groovy

+    static double getMaximumB(ShuntCompensator self) {
+        if (ShuntCompensatorModelType.LINEAR == self.getModelType()) {
+            // forward bPerSection * maximumSectionCount for linear shunts
+            return self.getModel(ShuntCompensatorLinearModel.class).getbPerSection() * self.getModel().getMaximumSectionCount()


Is it a bad idea to keep the getMaximumB in all shunt model and provide a convenient function?

The issue is that we used to have getMaximumB for linear shunts which returned the susceptance of the highest section (not the actual maximum susceptance) so if we put it in the shunt model, we would have several possibilities:

either keep this behavior for linear shunt but for non linear shunt it returns the actual maximum susceptance

or change the behavior and in any case, it returns the actual maximum susceptance

or keep this behavior for linear shunt and for non linear shunt it returns the susceptance of the highest section too

In any case, I find it too confusing... What do you think?

My opinion would be that it should return the actual maximum susceptance regardless of the model, as indicated by its name. Maybe another function could return the susceptance of the highest section for linear shunts if needed.

I agree with you, but isn't it deceitful to change the behavior of the method? Or I guess we could say that the previous one was buggy as when it was first implemented, it didn't take into account the case where the susceptance per section was negative...

miovd · 2020-03-18T11:09:30Z

Could you please check again if everything in the merging view is implemented? I think we should have adapter for the model since you have a backward link. Is it required to have this link?

You're right... I will think about it but I think I will have to implement an adapter for the shunt models.

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

agnesLeroy

@MioRtia thanks for your changes, it looks good to me now

sonarcloud · 2020-03-24T16:52:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

86.5% Coverage
0.0% Duplication

This reverts commit 7e9d21f. Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com> # Conflicts: # iidm/iidm-tck/src/test/java/com/powsybl/iidm/network/tck/AbstractShuntCompensatorTest.java

This reverts commit f60b5da.

This reverts commit f60b5da. Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

miovd added the Breaking Change API is broken label Mar 4, 2020

miovd added this to PR in progress in Evolutions IIDM via automation Mar 4, 2020

miovd changed the base branch from master to evolution_xiidm/1.2 March 4, 2020 08:06

miovd changed the base branch from evolution_xiidm/1.2 to master March 4, 2020 08:08

miovd force-pushed the new_shunt_api branch 3 times, most recently from dca9f3a to 8a332a1 Compare March 4, 2020 14:50

miovd changed the title ~~[WIP] Introduction of non linear shunt compensators~~ Introduction of non linear shunt compensators Mar 4, 2020

miovd added IIDM Doc needed labels Mar 4, 2020

miovd requested a review from mathbagu March 4, 2020 15:03

miovd added the PR: waiting-for-review label Mar 4, 2020

mathbagu reviewed Mar 5, 2020

View reviewed changes

iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/ShuntCompensatorLinearModelImpl.java Show resolved Hide resolved

...cripting/src/main/groovy/com/powsybl/iidm/network/scripting/ShuntCompensatorExtension.groovy Outdated Show resolved Hide resolved

miovd force-pushed the new_shunt_api branch from 8610ee5 to 44ec378 Compare March 5, 2020 13:30

miovd requested review from mathbagu, agnesLeroy and geofjamg March 5, 2020 14:11

agnesLeroy reviewed Mar 6, 2020

View reviewed changes

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/ShuntCompensatorModel.java Show resolved Hide resolved

agnesLeroy reviewed Mar 6, 2020

View reviewed changes

iidm/iidm-api/src/main/java/com/powsybl/iidm/network/ShuntCompensatorNonLinearModel.java Show resolved Hide resolved

mathbagu reviewed Mar 6, 2020

View reviewed changes

miovd force-pushed the new_shunt_api branch from 44ec378 to af7c349 Compare March 9, 2020 12:31

miovd requested a review from mathbagu March 9, 2020 13:46

miovd added the PR: conflict-with-main label Mar 12, 2020

miovd force-pushed the new_shunt_api branch from af7c349 to 118e4c8 Compare March 12, 2020 09:16

miovd removed the PR: conflict-with-main label Mar 12, 2020

mathbagu reviewed Mar 13, 2020

View reviewed changes

mathbagu added the PR: waiting-for-info-author label Mar 13, 2020

miovd force-pushed the new_shunt_api branch from 118e4c8 to b120df2 Compare March 18, 2020 10:30

miovd removed the PR: waiting-for-review label Mar 18, 2020

miovd added 7 commits March 24, 2020 15:01

add test

f82c4d2

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

Change shunt impl design + getMaximumSectionCount in Shunt API

50b704b

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

Corrections after rebasing master

c872863

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

Add merging view adapter for shunt models' adders

4abe617

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

corrections

0ab6019

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

add tests

106bab2

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

Attach the model to the shunt in the constructor

eb35f46

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

miovd force-pushed the new_shunt_api branch from 9b23d71 to eb35f46 Compare March 24, 2020 14:21

miovd mentioned this pull request Mar 24, 2020

Update documentation to new shunt API powsybl/powsybl.github.io#125

Merged

2 tasks

miovd removed the Doc needed label Mar 24, 2020

mathbagu requested a review from agnesLeroy March 24, 2020 15:22

agnesLeroy approved these changes Mar 24, 2020

View reviewed changes

miovd added PR: ready-to-be-merged and removed PR: corrected PR: waiting-for-review labels Mar 24, 2020

Merge branch 'master' into new_shunt_api

0456c76

miovd merged commit 7e9d21f into master Mar 24, 2020

Evolutions IIDM automation moved this from PR in progress to Merged PR Mar 24, 2020

miovd deleted the new_shunt_api branch March 24, 2020 16:55

miovd mentioned this pull request Mar 25, 2020

[IIDM 1.2] Introduction to non linear shunt powsybl/powsybl-iidm4cpp#69

Closed

miovd mentioned this pull request Apr 20, 2020

Revert non linear shunts #1285

Merged

4 tasks

miovd added a commit that referenced this pull request Apr 21, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

5229472

This reverts commit f60b5da.

miovd added a commit that referenced this pull request Jun 11, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

934739b

This reverts commit f60b5da.

miovd added a commit that referenced this pull request Jun 12, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

4d6000d

This reverts commit f60b5da.

miovd added a commit that referenced this pull request Jun 12, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

4731715

This reverts commit f60b5da.

miovd added a commit that referenced this pull request Jun 24, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

ca379d6

This reverts commit f60b5da. Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

miovd added a commit that referenced this pull request Jun 25, 2020

Revert "Revert "Introduction of non linear shunt compensators (#1202)""

c3d7ca5

This reverts commit f60b5da. Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduction of non linear shunt compensators #1202

Introduction of non linear shunt compensators #1202

miovd commented Mar 4, 2020 •

edited

mathbagu left a comment

mathbagu Mar 6, 2020

agnesLeroy Mar 6, 2020

miovd Mar 9, 2020 •

edited

miovd Mar 9, 2020 •

edited

mathbagu Mar 6, 2020

miovd Mar 9, 2020

miovd Mar 9, 2020 •

edited

mathbagu Mar 6, 2020

miovd Mar 9, 2020

mathbagu Mar 6, 2020

miovd Mar 9, 2020

mathbagu left a comment

mathbagu Mar 13, 2020

miovd Mar 18, 2020

agnesLeroy Mar 18, 2020

miovd Mar 18, 2020

miovd commented Mar 18, 2020

agnesLeroy left a comment

sonarcloud bot commented Mar 24, 2020


		ShuntCompensatorAdder setMaximumSectionCount(int maximumSectionCount);
		ShuntCompensatorNonLinearModelAdder newNonLinearModel();


		<M extends ShuntCompensatorModel> M getModel(Class<M> modelType);

		void setShuntCompensator(ShuntCompensatorImpl shuntCompensator);

Introduction of non linear shunt compensators #1202

Introduction of non linear shunt compensators #1202

Conversation

miovd commented Mar 4, 2020 • edited

mathbagu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miovd Mar 9, 2020 • edited

Choose a reason for hiding this comment

miovd Mar 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miovd Mar 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathbagu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miovd commented Mar 18, 2020

agnesLeroy left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 24, 2020

miovd commented Mar 4, 2020 •

edited

miovd Mar 9, 2020 •

edited

miovd Mar 9, 2020 •

edited

miovd Mar 9, 2020 •

edited