Clean CSA constants #1127

bqth29 · 2024-09-09T14:07:43Z

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?
Cleaner way to import CSA profiles with a generic NC parser

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?

Yes
No

If yes, please check if the following requirements are fulfilled

The Breaking Change or Deprecated label has been added
The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

Signed-off-by: Thomas Bouquet <[email protected]>

…e-constants # Conflicts: # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/cnec/AbstractCnecCreator.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/cnec/FlowCnecCreator.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/constants/CsaProfileConstants.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/nc/AssessedElement.java

Signed-off-by: Thomas Bouquet <[email protected]>

# Conflicts: # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/constants/CsaProfileConstants.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/remedialaction/CsaProfileRemedialActionsCreator.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/nc/TapChanger.java # data/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/nc/TapPositionAction.java

Signed-off-by: Thomas Bouquet <[email protected]>

benrejebmoh · 2024-10-29T11:02:36Z

...java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/cnec/CsaProfileCnecCreator.java

@@ -100,24 +100,16 @@ private void addCnec(AssessedElement nativeAssessedElement, Set<AssessedElementW
        }

        // We check whether the AssessedElement is defined using an OperationalLimit
-        LimitType limitType = getLimit(nativeAssessedElement);
+        Class<? extends Cnec<?>> limitType = getCnecType(nativeAssessedElement);


Suggested change

Class<? extends Cnec<?>> limitType = getCnecType(nativeAssessedElement);

if (nativeVoltageLimitPerId.get(nativeAssessedElement.operationalLimit()) != null) {

new VoltageCnecCreator(crac, network, nativeAssessedElement, nativeVoltageLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addVoltageCnecs();

} else if (nativeVoltageAngleLimitPerId.get(nativeAssessedElement.operationalLimit()) != null) {

new AngleCnecCreator(crac, network, nativeAssessedElement, nativeVoltageAngleLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addAngleCnecs();

} else {

// AssessedElement defined with a CurrentLimit or a conducting equipment

new FlowCnecCreator(crac, network, nativeAssessedElement, nativeCurrentLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addFlowCnecs();

}

benrejebmoh · 2024-10-30T09:29:23Z

...sybl/openrao/data/cracio/csaprofiles/craccreator/remedialaction/ElementaryActionsHelper.java

@@ -142,10 +141,10 @@ private String getAssociatedRemedialActionScheme(String remedialActionId) {
        }

        RemedialActionScheme nativeRemedialActionScheme = linkedRemedialActionSchemePropertyBags.get(0);
-        if (!CsaProfileConstants.SIPS.equals(nativeRemedialActionScheme.kind())) {
+        if (!"RemedialActionSchemeKind.sips".equals(nativeRemedialActionScheme.kind())) {


for better readability and maintainability -> to the top of class as a constant

phiedw · 2024-10-30T09:49:06Z

...o-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/CsaProfileCrac.java

+        if (queriedNativeObjects.containsKey(nativeType)) {
+            return (Set<T>) queriedNativeObjects.get(nativeType);
+        }
+        Query query = Arrays.stream(Query.values()).filter(q -> nativeType.equals(q.getNativeClass())).findFirst().orElseThrow();


Maybe you can put this in a static method of query (that would return an optional)

phiedw · 2024-10-30T09:55:00Z

...o-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/CsaProfileCrac.java

    private static boolean checkTimeCoherence(PropertyBag header, OffsetDateTime offsetDateTime) {
-        String startTime = header.getId(CsaProfileConstants.REQUEST_HEADER_START_DATE);
-        String endTime = header.getId(CsaProfileConstants.REQUEST_HEADER_END_DATE);
+        String startTime = header.getId(CsaProfileConstants.START_DATE);
+        String endTime = header.getId(CsaProfileConstants.END_DATE);
        return CsaProfileCracUtils.isValidInterval(offsetDateTime, startTime, endTime);
    }


Can you not replace this method with CsaProfileCracUtils.checkProfileValidityInterval directly?

phiedw · 2024-10-31T09:24:13Z

...-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/NativeParser.java

+        return propertyBag.getId(propertyBagName);
+    }
+
+    public static <T extends NCObject> T fromPropertyBag(PropertyBag propertyBag, Class<T> nativeClass, Map<String, Object> defaultValues) throws IllegalArgumentException, OpenRaoException, NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {


Is there a way to reduce the number of possible thrown exceptions?

phiedw · 2024-10-31T09:28:05Z

...-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/NativeParser.java

+    }
+
+    private static String getMrid(PropertyBag propertyBag, Class<?> nativeClass) {
+        String propertyBagName = Character.toLowerCase(nativeClass.getSimpleName().charAt(0)) + nativeClass.getSimpleName().substring(1);


can you not simply do getSimpleName().toLowerCase() or do you need any other upper case to stay upper case?

phiedw · 2024-10-31T10:02:14Z

...sa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/Query.java

+    private final Class<? extends NCObject> nativeClass;
+    private final Map<String, Object> defaultValues;
+
+    Query(Class<? extends NCObject> nativeClass, CsaProfileKeyword targetProfilesKeyword, OverridableAttribute overridableAttribute, Map<String, Object> defaultValues) {


Why is there only one OverridableAttribute? Also can we not always set a default vaule for each overridable attribute?
Depending on the answers maybe we can remove the third argument and know the all keys in the map are the overridableAttributes.

phiedw · 2024-10-31T10:22:52Z

...-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/NativeParser.java

+    public static <T extends NCObject> T fromPropertyBag(PropertyBag propertyBag, Class<T> nativeClass, Map<String, Object> defaultValues) throws IllegalArgumentException, OpenRaoException, NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
+        // Ensure the class is a record
+        if (!nativeClass.isRecord()) {
+            throw new IllegalArgumentException("Provided class is not a record");
+        }
+
+        // Get the record's components (fields)
+        RecordComponent[] components = nativeClass.getRecordComponents();
+
+        // Prepare an array to hold the values for the record's constructor
+        Object[] constructorArgs = new Object[components.length];
+
+        // Loop through the components and extract values from the property bag
+        for (int i = 0; i < components.length; i++) {
+            RecordComponent component = components[i];
+            if ("mrid".equals(component.getName())) {
+                constructorArgs[i] = getMrid(propertyBag, nativeClass);
+            } else {
+                Object parsedValue = parseStringValue(propertyBag.getId(component.getName()), component.getType());
+                if (parsedValue == null) {
+                    parsedValue = defaultValues.get(component.getName());
+                }
+                constructorArgs[i] = parsedValue;
+            }
+        }
+
+        // Find the canonical constructor of the record class
+        Constructor<T> constructor = nativeClass.getDeclaredConstructor(
+            // Get the types of the components (this matches the constructor signature)
+            Arrays.stream(components)
+                .map(RecordComponent::getType)
+                .toArray(Class<?>[]::new)
+        );
+
+        // Create and return a new instance of the record
+        return constructor.newInstance(constructorArgs);
+    }


This class is very complex and difficult to maintain. It also requires the name of the fields of the records to match perfectly the ones in the xsd. It also changes all the booleans into Booleans which makes the rest of the code a bit less nice (I guess this can be solved by changing the fields in the records to booleans?).

phiedw · 2024-10-31T10:45:43Z

...a/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/constants/CsaProfileConstants.java

+    public static final String START_DATE = "startDate";
+    public static final String END_DATE = "endDate";
+    public static final String KEYWORD = "keyword";


Either rename the class or find somewhere to store the remaining constants, now that most constants are in other classes

phiedw · 2024-10-31T10:55:23Z

...rao/data/cracio/csaprofiles/craccreator/remedialaction/CsaProfileRemedialActionsCreator.java

                    throw new OpenRaoImportException(ImportStatus.NOT_FOR_RAO, String.format("Remedial action %s will not be imported because normalAvailable is set to false", nativeRemedialAction.mrid()));
                }
                String elementaryActionsAggregatorId = isSchemeRemedialAction ? elementaryActionsHelper.getGridStateAlterationCollection(nativeRemedialAction.mrid()) : nativeRemedialAction.mrid(); // collectionIdIfAutoOrElseRemedialActionId
                RemedialActionType remedialActionType = getRemedialActionType(nativeRemedialAction.mrid(), elementaryActionsAggregatorId, isSchemeRemedialAction);
                RemedialActionAdder<?> remedialActionAdder;
                if (remedialActionType.equals(RemedialActionType.NETWORK_ACTION)) {
-                    remedialActionAdder = networkActionCreator.getNetworkActionAdder(elementaryActionsHelper.getTopologyActions(isSchemeRemedialAction), elementaryActionsHelper.getRotatingMachineActions(isSchemeRemedialAction), elementaryActionsHelper.getShuntCompensatorModifications(isSchemeRemedialAction), elementaryActionsHelper.getNativeStaticPropertyRangesPerNativeGridStateAlteration(), nativeRemedialAction.mrid(), elementaryActionsAggregatorId, alterations);
+                    remedialActionAdder = networkActionCreator.getNetworkActionAdder(elementaryActionsHelper.getTopologyActions(isSchemeRemedialAction).getOrDefault(elementaryActionsAggregatorId, Set.of()), elementaryActionsHelper.getRotatingMachineActions(isSchemeRemedialAction).getOrDefault(elementaryActionsAggregatorId, Set.of()), elementaryActionsHelper.getShuntCompensatorModifications(isSchemeRemedialAction).getOrDefault(elementaryActionsAggregatorId, Set.of()), elementaryActionsHelper.getNativeStaticPropertyRangesPerNativeGridStateAlteration(), nativeRemedialAction.mrid(), alterations);


maybe it's worth adding an argument in the elementaryActoinsHelper methods to pass the elementaryActionsAggregatorId

...sybl/openrao/data/cracio/csaprofiles/craccreator/remedialaction/ElementaryActionsHelper.java

...ofiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/nc/GridStateAlteration.java

# Conflicts: # data/crac/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/CsaProfileCracCreator.java # data/crac/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/cnec/FlowCnecInstantHelper.java # data/crac/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/craccreator/remedialaction/OnConstraintUsageRuleHelper.java # data/crac/crac-io/crac-io-csa-profiles/src/main/java/com/powsybl/openrao/data/cracio/csaprofiles/nc/AssessedElement.java

Signed-off-by: Thomas Bouquet <[email protected]>

bqth29 added 10 commits September 6, 2024 15:45

Import TapPositionAction from TapChanger

c3cee7f

Signed-off-by: Thomas Bouquet <[email protected]>

Author + copyright

bf7d37f

Signed-off-by: Thomas Bouquet <[email protected]>

Use queries

ef7689e

Signed-off-by: Thomas Bouquet <[email protected]>

Unify contingency queries

906e5cc

Signed-off-by: Thomas Bouquet <[email protected]>

Override query more elegantly

3d6367c

Signed-off-by: Thomas Bouquet <[email protected]>

Use generic property bag parser for native objects

2c81ca6

Signed-off-by: Thomas Bouquet <[email protected]>

Introduce overridable attributes

88720d9

Signed-off-by: Thomas Bouquet <[email protected]>

CsaInstants enum

6ff6151

Signed-off-by: Thomas Bouquet <[email protected]>

Simplify LimitType enum

db05da9

Signed-off-by: Thomas Bouquet <[email protected]>

Lint

6a0bce4

Signed-off-by: Thomas Bouquet <[email protected]>

bqth29 added the cleaning This issue or pull request only concerns improving the overall state of the code label Sep 9, 2024

bqth29 marked this pull request as ready for review September 9, 2024 14:07

bqth29 added 3 commits September 9, 2024 16:38

Merge branch 'main' into experiment/csa-tap-changers

c0b5aae

Merge tap changer branch

de0e4f2

Signed-off-by: Thomas Bouquet <[email protected]>

bqth29 requested review from benrejebmoh and jipea September 9, 2024 14:49

bqth29 added the swe-csa This issue or pull request addresses a feature or a bug relative to the SWE CSA process label Sep 11, 2024

bqth29 changed the base branch from experiment/csa-tap-changers to main September 23, 2024 07:42

bqth29 and others added 9 commits September 23, 2024 09:46

Merge main

33f51df

Signed-off-by: Thomas Bouquet <[email protected]>

Remove LimitType enum

a2fecbf

Signed-off-by: Thomas Bouquet <[email protected]>

Remove NCAggregator

e0f9f19

Signed-off-by: Thomas Bouquet <[email protected]>

Linter and sonar

eb05d7a

Signed-off-by: Thomas Bouquet <[email protected]>

Sonar + refactoring

2c6d9a0

Signed-off-by: Thomas Bouquet <[email protected]>

Refactored Network Action Adder

17882d6

Signed-off-by: Thomas Bouquet <[email protected]>

Merge branch 'main' into feature/csa/reorganize-constants

c429dd7

Merge main

1c05409

Signed-off-by: Thomas Bouquet <[email protected]>

bqth29 mentioned this pull request Oct 9, 2024

[CSA] Import ARA from time to implement only #1144

Open

7 tasks

Merge branch 'main' into feature/csa/reorganize-constants

4c36524

Merge branch 'main' into feature/csa/reorganize-constants

a54a2cb

bqth29 added the PR: waiting-for-review This PR is waiting to be reviewed label Oct 14, 2024

Merge branch 'main' into feature/csa/reorganize-constants

32b056b

benrejebmoh requested changes Oct 30, 2024

View reviewed changes

phiedw requested changes Oct 31, 2024

View reviewed changes

phiedw added PR: waiting-for-correction This PR is waiting to be corrected by its author and removed PR: waiting-for-review This PR is waiting to be reviewed labels Nov 6, 2024

bqth29 and others added 4 commits November 7, 2024 09:26

merge main

4327b93

Signed-off-by: Thomas Bouquet <[email protected]>

resolve some comments

6bf2981

Signed-off-by: Thomas Bouquet <[email protected]>

Merge branch 'main' into feature/csa/reorganize-constants

e4f5fa3

bqth29 marked this pull request as draft December 20, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean CSA constants #1127

Clean CSA constants #1127

bqth29 commented Sep 9, 2024

benrejebmoh Oct 29, 2024

benrejebmoh Oct 30, 2024

phiedw Oct 30, 2024

phiedw Oct 30, 2024

phiedw Oct 31, 2024

phiedw Oct 31, 2024

phiedw Oct 31, 2024

phiedw Oct 31, 2024

phiedw Oct 31, 2024

phiedw Oct 31, 2024

-        Class<? extends Cnec<?>> limitType = getCnecType(nativeAssessedElement);
+        if (nativeVoltageLimitPerId.get(nativeAssessedElement.operationalLimit()) != null) {
+            new VoltageCnecCreator(crac, network, nativeAssessedElement, nativeVoltageLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addVoltageCnecs();
+        } else if (nativeVoltageAngleLimitPerId.get(nativeAssessedElement.operationalLimit()) != null) {
+            new AngleCnecCreator(crac, network, nativeAssessedElement, nativeVoltageAngleLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addAngleCnecs();
+        } else {
+            // AssessedElement defined with a CurrentLimit or a conducting equipment
+            new FlowCnecCreator(crac, network, nativeAssessedElement, nativeCurrentLimitPerId.get(nativeAssessedElement.operationalLimit()), new HashSet<>(combinableContingencies), csaProfileCnecCreationContexts, rejectedLinksAssessedElementContingency, cracCreationParameters, borderPerTso, borderPerEic).addFlowCnecs();
+        }

Clean CSA constants #1127

Are you sure you want to change the base?

Clean CSA constants #1127

Conversation

bqth29 commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment