powsybl-core icon indicating copy to clipboard operation
powsybl-core copied to clipboard

Improve security analysis API

Open flo-dup opened this issue 3 years ago • 3 comments

  • Do you want to request a feature or report a bug? Design bug

  • What is the current behavior? API somehow inconsistent

  • If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem Launch a security analysis with e.g. a list of contingency, current API requires to call the run with the maximum number of parameters. Getting the violation results is not that convenient: run returns a SecurityAnalysisReport, then you need to apply getResult(), then getPostContingencyResults() for instance.

  • What is the expected behavior? Analysis easy to launch/code:

List<Contingency> contingencies = List.of(...);
SecurityAnalysisResult result = SecurityAnalysis.find(provider)
        .run(network, contingencies);

The logs byte array can be an optional field of SecurityAnalysisResult

  • What is the motivation / use case for changing the behavior? Well designed API

flo-dup avatar Feb 09 '22 09:02 flo-dup

Note that there was already some discussion 1 year ago about the moving of logs inside SecurityAnalysisResult, for ex. here: https://github.com/powsybl/powsybl-core/pull/1585#discussion_r553945200.

We should probably discuss it again, but at the time we voluntarily chose to have a "report" which would provide more technical information about the execution of the security analysis, while the "business domain" information goes into the result.

One technical question was the handling of those bytes when serializing/deserializing results (to JSON).

sylvlecl avatar Feb 09 '22 18:02 sylvlecl

I was not aware of that discussion, sorry! Thanks Sylvain for pointing that out. Nonetheless I think we could discuss it again, as the technical information is destined to be one day replaced by reports within a reporter (and then the SecurityAnalysisReport would only contain the SecurityAnalysisResults). Besides the problem of confusion between SecurityAnalysisReport and Reporter remains.

I'll restructure the PR to revert that SecuirtyAnalysisReport - SecurityAnalysisResults merge and have only the run changes, which are not problematic.

flo-dup avatar Feb 10 '22 15:02 flo-dup

@sylvlecl Someone (but who? :zipper_mouth_face:) suggested lately to replace SecurityAnalysisReport by SecurityAnalysisExecutionResult to make it clearer. Would you agree on such a change?

Indeed the changes due to the Reporter might not be done before a while, so it would be nice to clarify a bit this Report/Result difference in the meantime don't you think?

flo-dup avatar Apr 14 '22 08:04 flo-dup