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

Security analysis runner parameters #3017

Merged
merged 13 commits into from
May 27, 2024
Merged

Security analysis runner parameters #3017

merged 13 commits into from
May 27, 2024

Conversation

Lisrte
Copy link
Contributor

@Lisrte Lisrte commented May 15, 2024 •
edited
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?

What kind of change does this PR introduce?

Refactor

What is the current behavior?

Security analysis API methods contains too many parameters and overloads

What is the new behavior (if this is a feature change)?
Replace the parameters with a new object : SecurityAnalysisRunParameters

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)

For SecurityAnalysisProvider implementations:
replace run(Network, String workingStateId, LimitViolationDetector, LimitViolationFilter, ComputationManager, SecurityAnalysisParameters, ContingenciesProvider, List<SecurityAnalysisInterceptor>, List<OperatorStrategy>, List<Action>, List<StateMonitor>, List<LimitReduction>, ReportNode
with run(Network, String workingStateId, ContingenciesProvider, SecurityAnalysisRunParameters)

For SecurityAnalysis API, the various run and runAsync methods overloads are now limited to:

  • run(Network, String workingStateId, ContingenciesProvider, SecurityAnalysisRunParameters)
  • runAsync(Network, List<Contingency>, SecurityAnalysisRunParameters)
  • runAsync(Network, List<Contingency>)

Other information:

Lisrte added 2 commits May 14, 2024 15:50
@Lisrte
Refactor Security analysis runner
6db790a 
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte
Refactor Security analysis runner
fd06e91 
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte Lisrte added the Breaking Change API is broken label May 15, 2024
@Lisrte Lisrte mentioned this pull request May 15, 2024
Merged
7 tasks
@Lisrte
Review fix
3e226c2 
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte Lisrte marked this pull request as ready for review May 17, 2024 13:29
@Lisrte Lisrte requested review from flo-dup and geofjamg May 17, 2024 13:29
flo-dup
flo-dup reviewed May 21, 2024
View reviewed changes
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.

Seems good to me! just a few javadoc comments

...lysis/security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisProvider.java Outdated Show resolved Hide resolved
.../security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisRunParameters.java Outdated Show resolved Hide resolved
.../security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisRunParameters.java Outdated Show resolved Hide resolved
.../security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisRunParameters.java Outdated Show resolved Hide resolved
.../security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisRunParameters.java Outdated Show resolved Hide resolved
.../security-analysis-api/src/main/java/com/powsybl/security/SecurityAnalysisRunParameters.java Outdated Show resolved Hide resolved
Lisrte and others added 3 commits May 22, 2024 11:20
@Lisrte @flo-dup
Update security-analysis/security-analysis-api/src/main/java/com/pows…
f7f2c84 
…ybl/security/SecurityAnalysisProvider.java

Co-authored-by: Florian Dupuy <66690739+flo-dup@users.noreply.github.com>
Signed-off-by: Lisrte <laurent.issertial@rte-france.com>
@Lisrte @flo-dup
Apply suggestions from code review
5b2f9cf 
Co-authored-by: Florian Dupuy <66690739+flo-dup@users.noreply.github.com>
Signed-off-by: Lisrte <laurent.issertial@rte-france.com>
@Lisrte
Review fix
43baa0c 
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte Lisrte requested a review from flo-dup May 22, 2024 11:20
flo-dup
flo-dup approved these changes May 22, 2024
View reviewed changes
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.

Seems good to me!

@geofjamg could you have a quick look?

And @EtienneLt or @geofjamg do you think you could add a clearer explanation for SecurityAnalysisRunParameters::setMonitors javadoc? 😬

It would be nice to know which information, where is it written? It's also confusing "after security analysis" seems to mean after all contingencies, but it's not.

annetill and others added 2 commits May 23, 2024 10:38
@annetill
Merge branch 'main' into sa_runner_parameter
d9ee814
@annetill
Improve javadoc.
f16201c 
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
flo-dup
flo-dup reviewed May 24, 2024
View reviewed changes
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.

Thanks a lot for the javadoc @annetill!!

...-analysis/security-analysis-api/src/main/java/com/powsybl/security/results/BranchResult.java Outdated Show resolved Hide resolved
...ity-analysis/security-analysis-api/src/main/java/com/powsybl/security/results/BusResult.java Outdated Show resolved Hide resolved
...-analysis-api/src/main/java/com/powsybl/security/results/ThreeWindingsTransformerResult.java Outdated Show resolved Hide resolved
...-analysis/security-analysis-api/src/main/java/com/powsybl/security/monitor/StateMonitor.java Outdated Show resolved Hide resolved
...-analysis-api/src/main/java/com/powsybl/security/results/ThreeWindingsTransformerResult.java Outdated Show resolved Hide resolved
...-analysis/security-analysis-api/src/main/java/com/powsybl/security/monitor/StateMonitor.java Outdated Show resolved Hide resolved
flo-dup added 3 commits May 24, 2024 15:09
@flo-dup
Merge branch 'main' into sa_runner_parameter
63ad828 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
More details in the javadoc
a7638d8 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Fix typo
7389743 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
geofjamg
geofjamg approved these changes May 27, 2024
View reviewed changes
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.

@Lisrte ok for me but don't forget before merging to add licence and authorship for newly added files

Lisrte added 2 commits May 27, 2024 10:11
@Lisrte
Add copyright
ec182bc 
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte
Merge branch 'main' into sa_runner_parameter
48aae37
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@olperr1 olperr1 merged commit 9f8546a into main May 27, 2024
6 checks passed
@olperr1 olperr1 deleted the sa_runner_parameter branch May 27, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annetill annetill annetill left review comments

@flo-dup flo-dup flo-dup approved these changes

@geofjamg geofjamg geofjamg approved these changes

Assignees
No one assigned
Labels
Breaking Change API is broken June release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Lisrte @geofjamg @annetill @flo-dup @olperr1