-
Notifications
You must be signed in to change notification settings - Fork 8
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
LfAction refactoring and area interchange target action support #1172
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
…alysis. Signed-off-by: Bertrand Rix <[email protected]>
…failure of applying an action. Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Note for the reviewer : There was no specific rule about crashing or outputing a warning when an action cannot be applied so I kept more or less the same behavior for each action. Some of the warning / exception raising have been moved from construction to action application though. In my opinion I would prefer to raise warning and eventually have something in the output API to query which action has been applied rather that raising an exception for potentially a single wrong action and losing all the analysis. |
Signed-off-by: Bertrand Rix <[email protected]>
Signed-off-by: Bertrand Rix <[email protected]>
Quality Gate passedIssues Measures |
To sum up a bit, there is 4 cases of action setup that causes an exception raise : • For TapChangerAction, if the PiModel is a SimpleModel (no tap changer on the transformer) -> throw UnsupportedOperationException Those exception are the same as before the refactoring. All other potential issue are only outputting a warning, for example a branch not found in the network for a TerminalsConnectionAction. I also added a global warning for failing to apply an action in LfActionUtils, which might lead to a lot of warning when doing a security analysis on multiple component. So not sure we should keep that. |
acParameters.getNetworkParameters().setAreaInterchangeControl(false); | ||
|
||
LfAction lfAreaTargetAction = LfActionUtils.createLfAction(targetAction, network, acParameters.getNetworkParameters().isBreakers(), lfNetwork); | ||
assertFalse(lfAreaTargetAction.apply(lfNetwork, null, acParameters.getNetworkParameters())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior should change very soon, after #1156 's merge.
Because of the ContingencyLoadFlowParameters extension, AIC can be performed on some post-contingency cases even if networkParameters.isAreaInterchangeControl() is false. So Area-s will now always be created, and this assertion will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I will update if 1156 is merged before this one.
src/main/java/com/powsybl/openloadflow/network/action/LfActionUtils.java
Show resolved
Hide resolved
private LfActionUtils() { | ||
} | ||
|
||
public static LfAction createLfAction(Action action, Network network, boolean breakers, LfNetwork lfNetwork) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don(t you return an optional ? The previous code could help detect NOOP actions for the network by returning an empty value if the element did not exist.
You now loose the ability to detect actions that do nothing on a given LfNetwork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code was returning an optional but actually did not do anything with the "empty" information and just silently ignored the action.
Same checks are done in the apply method with a boolean return and a warning is logged (to be discussed maybe).
Ultimately I think the operator strategy result API should hold the info of what action was applied successfully, this is also why I designed it like this, but this a change of API to be seen in the futur..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
But isn't there a functional change ?
In current code, I suppose there is no LF run if the list of LF actions is empty for a strategy. Is it the same here .
The point of reporting the missing action is a good point but:
Should be done in a report (users won't look at the log)
May not be relevant in the (rare ?) case of contingency and actions covering different network component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current code, even if the list of lfAction converted from iidm actions is empty there is always a load flow done and an operator strategy result is produced (see AbstractSecurityAnalysis.runSimulations usage of lfActionById).
Whether that is a good thing is probably a good question but there is no functionnal change here as far as I can see.
Furthermore we still have the checkActions method that do some additional checks and raise an exception if there is some inconsistency. This one is still there.
Ok for putting the missing action in the report, good idea!
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No issue
What kind of change does this PR introduce?
Add the support of AreaInterchangeTarget action and change the implementation of LfAction.
LfAction existing design was confusing as everything was handled in the unique LfAction object.
A proper class hierarchy has been setup so as the make feature extension and maintenance easier.
What is the current behavior?
All actions implemented in the LfAction object. Created through the create method and applied through the global update connectivity method (for switch and terminal connection actions) and apply method.
Each instance of LfAction was capable of describing all kind of actions through all the available attributes but a single instance was created per iidm action.
What is the new behavior (if this is a feature change)?
All action creation and application processes are implemented in dedicated class, with common code in AbstractLfAction for all actions and additional AbstractLfBranchAction for switch and terminal connection action (they both impact topology).
Applying the action can be done through a call to the apply method or through a call to LfActionUtils.applyListOfActions. This last method, in a similar way that was done before the refactoring, apply a list of action by first applying the actions modifying topology and then the rest of the actions.
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: