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.

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

stores and apply several limit sets in the branch creation #9

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Mathieu-Deharbe
Copy link

@Mathieu-Deharbe Mathieu-Deharbe commented Dec 10, 2024

Adds the selected operational limit groups to the quad creation. And turn the current limits on side1 and 2 into lists (containing the currently selected but also others).

Mathieu-Deharbe and others added 7 commits December 12, 2024 11:26
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
import lombok.experimental.SuperBuilder;

import java.util.List;

/**
* @author Sylvain Bouzols <sylvain.bouzols at rte-france.com>
* ~= environ operational limit group, ~= limit set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be removed

Copy link
Author

@Mathieu-Deharbe Mathieu-Deharbe Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I instead createa an OperationalLimitsGroup class to be closer to IIDM.

@@ -27,6 +25,9 @@
@Schema(description = "Current Limits")
public class CurrentLimitsInfos {

@Schema(description = "Operational limit group name")
private String operationalLimitGroupId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operationalLimitsGroupId ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also change the description

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class member has been removed. But yep the 's' was missing.

@@ -43,6 +43,11 @@
// TODO remove public qualifier for all methods
public final class ModificationUtils {

public enum Side {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this
you can use enum Side of powsyble

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I changed it, thank you.


@Schema(description = "Current limits Side 2")
private CurrentLimitsInfos currentLimits2;
@Schema(description = "Selected operational limits group id on Side 2")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selected operational limits group on Side 2

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK why not.

List<OperationalLimitsGroupInfos> opLimitsGroupSide1 = modificationInfos.getOperationalLimitsGroups1();
List<OperationalLimitsGroupInfos> opLimitsGroupSide2 = modificationInfos.getOperationalLimitsGroups2();
if (!CollectionUtils.isEmpty(opLimitsGroupSide1)) {
var line = ModificationUtils.getInstance().getLine(network, modificationInfos.getEquipmentId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var line = ModificationUtils.getInstance().getLine(network, modificationInfos.getEquipmentId());
Line line = ModificationUtils.getInstance().getLine(network, modificationInfos.getEquipmentId());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK done.

private List<OperationalLimitsGroupInfos> operationalLimitsGroups2;

@Schema(description = "Selected operational limits group id on Side 1")
private String selectedOperationalLimitsGroupId1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private String selectedOperationalLimitsGroup1;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK done.

.setValue(limit.getValue() == null ? Double.MAX_VALUE : limit.getValue())
.setAcceptableDuration(limit.getAcceptableDuration() == null ? Integer.MAX_VALUE : limit.getAcceptableDuration())
.endTemporaryLimit();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can suggest here to use the functional approach (with forEach)

       .forEach(limit -> {
           double value = limit.getValue() != null ? limit.getValue() : Double.MAX_VALUE;
           int duration = limit.getAcceptableDuration() != null ? limit.getAcceptableDuration() : Integer.MAX_VALUE;

           limitsAdder.beginTemporaryLimit()
                      .setName(limit.getName())
                      .setValue(value)
                      .setAcceptableDuration(duration)
                      .endTemporaryLimit();
       });
                   }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK done. And it reduces cognitive complexity. Nice.

* @param branch branch to which limits are going to be added
* @param side which side of the branch receives the limits
*/
public void setCurrentLimitsOnASide(List<OperationalLimitsGroupInfos> opLimitGroups, Branch<?> branch, TwoSides side) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setCurrentLimitsOnSide (don't like the LimitsOnASide)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ? I think this is more precise with "A" given that "side" also means "secondary" or "in the same time" in english.
At least here we know this is a noun.

var line = ModificationUtils.getInstance().getLine(network, modificationInfos.getEquipmentId());
ModificationUtils.getInstance().setCurrentLimits(currentLimitsInfos1, line.newCurrentLimits1());
ModificationUtils.getInstance().setCurrentLimits(currentLimitsInfos2, line.newCurrentLimits2());
ModificationUtils.getInstance().setCurrentLimitsOnASide(opLimitsGroupSide2, line, TWO);
}
ModificationUtils.getInstance().disconnectBranch(modificationInfos, network.getLine(modificationInfos.getEquipmentId()), subReportNode);
// properties
Line line = network.getLine(modificationInfos.getEquipmentId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can place this line before (for example, line 74) to avoid calling getLine three times and
extract the code of // Set permanent and temporary current limits... shared between line and twt into a common funct (the second remarq is a suggestion not mandatory)

Copy link
Author

@Mathieu-Deharbe Mathieu-Deharbe Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I thought about this but didn't dare because it looks like whoever wrote this like this did it on purpose. And I didn't want to take the risk.

But ok you look motivated I did it !

Mathieu-Deharbe and others added 6 commits January 6, 2025 18:26
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants