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

Introduce ScalingParameters for Scalable #2461

Merged
merged 10 commits into from
Mar 15, 2023
Merged

Conversation

colinepiloquet
Copy link
Member

Signed-off-by: Coline PILOQUET coline.piloquet@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
No

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix: adds a boolean saying whether the terminal should be reconnected in case of scaling on a disconnected element.

What is the current behavior? (You can also link to an open issue here)
The terminal is always reconnected.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 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

83.3% 83.3% Coverage
0.0% 0.0% Duplication

@@ -109,7 +109,7 @@ public void filterInjections(Network n, List<Injection> injections, List<String>
* If scalingConvention is GENERATOR, the load active power decreases for positive "asked" and increases inversely
*/
@Override
public double scale(Network n, double asked, Scalable.ScalingConvention scalingConvention) {
public double scale(Network n, double asked, Scalable.ScalingConvention scalingConvention, boolean reconnect) {
Copy link
Member

Choose a reason for hiding this comment

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

We really should consider grouping all theses parameters in a ScalingContext object.

Signed-off-by: Coline PILOQUET <coline.piloquet@rte-france.com>
This reverts commit 8e4c410.

Signed-off-by: Coline PILOQUET <coline.piloquet@rte-france.com>
Signed-off-by: Coline PILOQUET <coline.piloquet@rte-france.com>
@colinepiloquet colinepiloquet force-pushed the fix_connect_scalable_optional branch from defa7e5 to bda3d70 Compare March 13, 2023 14:28
Signed-off-by: Coline PILOQUET <coline.piloquet@rte-france.com>
*/
public static ScalingContext getDefault() {
return new ScalingContext(Scalable.ScalingConvention.GENERATOR, false, false);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if it is the right design but it seems useful to have a default context

@colinepiloquet colinepiloquet requested a review from geofjamg March 13, 2023 15:02
@@ -109,9 +109,9 @@ public void filterInjections(Network n, List<Injection> injections, List<String>
* If scalingConvention is GENERATOR, the load active power decreases for positive "asked" and increases inversely
*/
@Override
public double scale(Network n, double asked, Scalable.ScalingConvention scalingConvention) {
public double scale(Network n, double asked, ScalingContext context) {
Copy link
Member

Choose a reason for hiding this comment

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

If we just add read only data in ScalingContext, it is not really a context. It is more a ScalingParameter

colinepiloquet and others added 2 commits March 14, 2023 12:27
Signed-off-by: Coline PILOQUET <coline.piloquet@rte-france.com>
@colinepiloquet colinepiloquet requested a review from annetill March 14, 2023 11:30
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@annetill annetill changed the title [Fix] Make reconnecting terminal optional in scalable. Introduce ScalingParameters for Scalable Mar 14, 2023
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@miovd miovd added the Breaking Change API is broken label Mar 15, 2023
@sonarqubecloud
Copy link

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.7% 87.7% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

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

Nice, just a tiny javadoc comment!

@@ -134,43 +134,10 @@ enum ScalingConvention {
* @param asked value asked to adjust the scalable active power
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add param annotation in the javadoc for the new parameter

@miovd miovd merged commit f4c582f into main Mar 15, 2023
@miovd miovd deleted the fix_connect_scalable_optional branch March 15, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants