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

manage additional factors #265

Merged
merged 6 commits into from
Apr 21, 2021
Merged

manage additional factors #265

merged 6 commits into from
Apr 21, 2021

Conversation

Djazouli
Copy link
Contributor

@Djazouli Djazouli commented Apr 7, 2021

Signed-off-by: Gael Macherel gael.macherel@artelys.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)

@Djazouli Djazouli requested review from geofjamg and annetill April 7, 2021 13:23
Base automatically changed from V_sensitivity to master April 8, 2021 09:10
@Djazouli Djazouli force-pushed the additional_factors branch 2 times, most recently from 3787fa4 to 737f518 Compare April 9, 2021 08:27
Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
@Djazouli Djazouli force-pushed the additional_factors branch from 737f518 to 335b624 Compare April 12, 2021 16:39
# Conflicts:
#	src/test/java/com/powsybl/openloadflow/sensi/dc/DcSensitivityAnalysisContingenciesTest.java
Copy link
Member

@geofjamg geofjamg left a comment

Choose a reason for hiding this comment

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

Looks good to me, the way it is implemented is very clear and simple.
I am just wondering if there is a better design for ContingencyContext. Marker interfaces are often not considered as a good practice, but not easy find an other simple design.

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.

Neat implementation - as already said above! Just a couple of remarks

Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
…bl-open-loadflow into additional_factors

Signed-off-by: Gael Macherel <gael.macherel@artelys.com>
@@ -46,50 +46,54 @@ public SensitivityFactorReaderAdapter(Network network, SensitivityFactorsProvide

@Override
public void read(Handler handler) {
ContingencyContext commonContingencyContext = new ContingencyContext(ContingencyContextType.ALL, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as small improvement, would be better to use 3 static methods to create ContingencyContexts (ContingencyContext.createAllContingencyContext(), ContingencyContext.createNoneContingencyContext(), ContingencyContext.createSpecificContingencyContext(String id))

Djazouli and others added 2 commits April 20, 2021 14:44
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

98.9% 98.9% Coverage
0.0% 0.0% Duplication

@geofjamg geofjamg merged commit d19aeb2 into master Apr 21, 2021
@geofjamg geofjamg deleted the additional_factors branch April 21, 2021 07:55
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.

4 participants