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

Dynawo models supplier #349

Merged
merged 15 commits into from
Jun 7, 2024
Merged

Dynawo models supplier #349

merged 15 commits into from
Jun 7, 2024

Conversation

Lisrte
Copy link
Contributor

@Lisrte Lisrte commented May 15, 2024

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?

Feature

What is the current behavior?

The only DynamicModelsSupplier implementation come from powsybl-core and use groovy script as input (same for event).

What is the new behavior (if this is a feature change)?
Add a new implementation using DynamicModelConfig from JSON file.

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: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>

private static void invokeMethod(Class<? extends ModelBuilder> builderClass, ModelBuilder<DynamicModel> builder, Property property) {
try {
builderClass.getMethod(property.name(), property.propertyClass()).invoke(builder, property.value());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep in mind that this might take time and that we could maybe cache the getMethod?

Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Add javadoc

Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte Lisrte marked this pull request as ready for review May 22, 2024 11:28
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@gautierbureau
Copy link
Member

@flo-dup any news on this as @thangqp will need this to continue his work

return builder.build();
}

private static void parseArrays(JsonParser parser, PropertyBuilder builder) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

You tried to reuse JsonUtil methods, so why didn't you use JsonUtil::parseValueArray here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JsonUtil::parseValueArray does not work since we try to parse array of arrays. The method expects a value when we want to parse internal array's parenthesis.

@@ -21,7 +21,7 @@
*/
public class TapChangerAutomationSystemBuilder extends AbstractAutomationSystemModelBuilder<TapChangerAutomationSystemBuilder> {

private static final String CATEGORY = "tapChangers";
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason of changing the case?

Copy link
Contributor Author

@Lisrte Lisrte Jun 5, 2024

Choose a reason for hiding this comment

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

In the first version of this PR, the category field became accessible to user and the lower case and plural form did not seem to match the 'unique category' meaning it was supposed to be.

Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
Signed-off-by: lisrte <laurent.issertial@rte-france.com>
@Lisrte Lisrte requested a review from flo-dup June 5, 2024 14:14
Copy link

sonarcloud bot commented Jun 7, 2024

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