Update voltage initializer to open reac version 0.3

powsybl / pypowsybl

A PowSyBl and Python integration based on GraalVM native image

Mozilla Public License 2.0

55 stars 12 forks source link

Update voltage initializer to open reac version 0.3 #671

Closed pjeanmarie closed 11 months ago

pjeanmarie commented 11 months ago

…ith new java API for addSpecificVoltageLimits

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?

What is the current behavior?

What is the new behavior (if this is a feature change)?

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

[ ] 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:

pjeanmarie commented 11 months ago

Question: I have updated create_voltage_limit_override (using two fonctions as for add_specific_voltage_limits) but is it used? Maybe create_voltage_limit_override need to be removed instead of updated

@geofjamg We will supress create_voltage_limit_override which was created in https://github.com/powsybl/pypowsybl/pull/627, is it a problem?

pjeanmarie commented 11 months ago

Notes about chosen solution: It allows to:

give only low or only high limit to a voltage level as it is now possible with the new java api
use the new "is relative" parameter
instead of the java API addSpecificVoltageLimits which have enum (LOW_VOLTAGE_LIMIT, HIGH_VOLTAGE_LIMIT) as a parameter, the python API is splitted in two functions add_specific_low_voltage_limits and add_specific_high_voltage_limits

pjeanmarie commented 11 months ago

(Integration) testing?

pjeanmarie commented 11 months ago

WIP -> Missing one test which will check that the values send by the new API are taken into account in the java network

pjeanmarie commented 11 months ago

Do not know how to really test this feature, all VoltageInitializerParameters.run() seems to crash with error "Module ampl is missing in the configuration file" for all given networks. It was unseen because the test VoltageInitializerParameters.run is skipped in the main, so no test in main. However, I can go from "At least one voltage level has an undefined or incorrect voltage limit." error to the error above by adding missing voltage level limits with the new API, so it "works".

obrix commented 11 months ago

@pjeanmarie For deleting create_voltage_limit_override I think it is okay, it returns a JavaHandle for a VoltageLimitOverride but there is no use for it because there is not API taking it. This may have been a useless function that we missed in the initial review. @EtienneLt Are you okay with that ?

EtienneLt commented 11 months ago

@pjeanmarie For deleting create_voltage_limit_override I think it is okay, it returns a JavaHandle for a VoltageLimitOverride but there is no use for it because there is not API taking it. This may have been a useless function that we missed in the initial review. @EtienneLt Are you okay with that ?

ok for me