-
Notifications
You must be signed in to change notification settings - Fork 51
Have multiple active operational limits group #3735
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
base: main
Are you sure you want to change the base?
Have multiple active operational limits group #3735
Conversation
…itsGroup Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…lected Signed-off-by: Nathan Dissoubray <[email protected]>
…le on TATL Also deprecate the methods that return only one overload for TATL TODO: do the same for the PATL Signed-off-by: Nathan Dissoubray <[email protected]>
…t from LimitsComputer interface and impl Signed-off-by: Nathan Dissoubray <[email protected]>
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/FlowsLimitsHolder.java
Outdated
Show resolved
Hide resolved
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/FlowsLimitsHolder.java
Outdated
Show resolved
Hide resolved
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/FlowsLimitsHolder.java
Outdated
Show resolved
Hide resolved
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/FlowsLimitsHolder.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
| public void setSelectedOperationalLimitsGroup(String id) { | ||
| setSelectedOperationalLimitsGroupNullableId(Objects.requireNonNull(id)); | ||
| Objects.requireNonNull(id); | ||
| //TODO this is for the notifyUpdate, but is it even relevant with the new impl ? |
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.
Yes, we should keep the notifications.
Little side-note, the content of these notifications will change (2 lists instead of 2 single Strings). I think we cannot avoid this breaking change.
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.
Indeed, removed the TODO in cc5aa65
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…mitsGroupsImpl Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…deprecate them Signed-off-by: Nathan Dissoubray <[email protected]>
Co-authored-by: Olivier Perrin <[email protected]> Signed-off-by: NathanDissoubray <[email protected]>
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/Branch.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImpl.java
Outdated
Show resolved
Hide resolved
| overload.getLimitReductionCoefficient(), | ||
| value, | ||
| side)); | ||
| overloadOnTemporary = true; |
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.
There's a little "trick" here.
We want to have one or zero LimitViolation per group: if a violation is detected for a temporary limit, we don't check the permanent limit (a temporary limit violation is more critical than the permanent limit violation).
We should keep the same policy for each group. Detecting a temporary limit violation for a group must not block the detection of a permanent limit violation for another group.
Therefore, we cannot rely on a single boolean overloadOnTemporary to determine if the detection on the permanent limits of the different groups is to be performed.
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.
yes that goes with the TODO just below, which asks if I should change the LimitViolationUtils.checkPermanentLimit to a something like a LimitViolationUtils.checkAllSelectedPermanentLimit (in case the permanent limits are different between groups, which could be the case)
to solve this, it would help if we had the id of the group in the overload, that way instead of a single boolean, we make a list of the groups that have overloaded on a temporary, and we only check the permanent for the other
This also leads to the question of: should it be checkAllSelectedPermanentLimit(branch, side, type, computer) or if it should be checkPermanentLimitFromGroupId (branch, side, groupId, type, computer) (so we don't check the permanent limit then filter out groups that have crossed a temporary, but just check for groups we know haven't crossed a temporary)
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.
For the Overload, yes I think it would be easier to also have the groupId in it.
There's 2 possible ways for your second paragraph:
- you check all permanent limits, then discard the detected permanent limit overloads for each group for which a temporary limit overload was already detected
- or, you retrieve all the ids of the selected groups and check the permanent limits for all of them for which no temporary limit overload was previously detected.
I think that the second option should be slightly more performant.
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 second option should be more performant indeed, but that would imply creating a function in LimitsComputer to retrieve the LimitsContainer when given a group id. And implementing that function in the class related to that interval.
If that's acceptable we can do that.
Otherwise, if we do not want the first method, another solution is to not check temporary and permanent separately in checkLimitViolation.
In the LimitViolationsUtils.checkAllTemporaryLimits, we do:
return limitsComputer.computeLimits(branch, type, side.toThreeSides(), false)
.stream()
.map(limits -> getOverload(limits, i))
.toList();instead of calling that function to get a collection of overload related to the temporary limits in checkLimitViolation,
In LimitViolationDetection.checkLimitViolation, we can do:
- get the
Collection<LimitsContainer<L>>from the computeLimits - make a copy of that
- on the original, do the same as checkAllTemporaryLimits by using
getOverload - now filter the copy to remove the LimitsContainer whose id correspond to an overload
- do the detection of the permanent limits like in
LimitViolationsUtils.checkPermanentLimit, ie :
.map(l -> checkPermanentLimitIfAny(l, i))
.orElse(new PermanentLimitCheckResult(false, 1.0))It would mean copying part of the code of those Utils functions (which could be less clean, but those utils functions are used only once overall, by the checkLimitViolation). If we do, that means we do not need to create new functions in LimitsComputer, and we don't double check limit groups where we know we already have an overload on a TATL (so it's still performant)
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.
You don't need to make a copy of the LimitsContainer's collection. You can create 2 streams on it since none will modify the collection.
But yes, having a single call to the computeLimits is a good idea!
…sGroupImpl Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…eady selected Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Olivier Perrin <[email protected]>
| case LINE -> ((Line) identifiable).getAllSelectedLimits(limitType, side.toTwoSides()); | ||
| case TIE_LINE -> ((TieLine) identifiable).getAllSelectedLimits(limitType, side.toTwoSides()); | ||
| case TWO_WINDINGS_TRANSFORMER -> ((TwoWindingsTransformer) identifiable).getAllSelectedLimits(limitType, side.toTwoSides()); |
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.
These 3 cases can be grouped into a single one. getAllSelectedLimits(...) is defined in Branch and the 3 types inherit it.
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.
done in f11157f
but I noticed getLoadingLimits and getAllSelectedLoadingLimits are actually not used
getLoadingLimits already exists so it could be used outside, which means removing it would be a breaking change, but getAllSelectedLoadingLimits is not there yet, do we not introduce it with this PR ? do we keep it to be coherent with getLoadingLimits ?
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.
Yes, I think we should have the getAllSelectedLoadingLimits method.
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/util/LimitViolationUtils.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupsImpl.java
Outdated
Show resolved
Hide resolved
| overload.getLimitReductionCoefficient(), | ||
| value, | ||
| side)); | ||
| overloadOnTemporary = true; |
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.
For the Overload, yes I think it would be easier to also have the groupId in it.
There's 2 possible ways for your second paragraph:
- you check all permanent limits, then discard the detected permanent limit overloads for each group for which a temporary limit overload was already detected
- or, you retrieve all the ids of the selected groups and check the permanent limits for all of them for which no temporary limit overload was previously detected.
I think that the second option should be slightly more performant.
iidm/iidm-impl/src/test/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImplTest.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/test/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImplTest.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/OperationalLimitsGroupImpl.java
Outdated
Show resolved
Hide resolved
…switch case Also remove the unchecked type cast Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…pl and test Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…nch directly No need to do the same for the T3W, since the legs are already FlowsLimitsHolder, meaning we can do t3w.getLeg(side).getAllSelectedOperationalLimitsGroupIds() Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…them return optional instead of potential null Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
…tionDetection Signed-off-by: Nathan Dissoubray <[email protected]>
Technically, it doesn't do the same thing when looking at the values used. For the test case it does, but that might be luck Signed-off-by: Nathan Dissoubray <[email protected]>
…Detection Signed-off-by: Nathan Dissoubray <[email protected]>
…rmanentLimitCheckResult Signed-off-by: Nathan Dissoubray <[email protected]>
Signed-off-by: Nathan Dissoubray <[email protected]>
|


Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
Closes #3703
What kind of change does this PR introduce?
Adds the ability to have multiple operational limits group active on a single equipment at the same time.
What is the current behavior?
Currently, only one can be active at a time.
What is the new behavior (if this is a feature change)?
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)
TODO once reviews are done
Other information:
Still need to modify
SecurityandSerDe, opening as draft for now