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

EMF module and merge tests #82

Merged
merged 7 commits into from
Jan 31, 2023
Merged

EMF module and merge tests #82

merged 7 commits into from
Jan 31, 2023

Conversation

obrix
Copy link
Member

@obrix obrix commented Jan 23, 2023

Add emf module (empty for now) with three tests for destructive merge, merging view merge and merge from CGMES loader.
EMF test written from powsybl tutorial to be done.

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • 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 ? If so, link to this issue using '#XXX' and skip the rest

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
@obrix obrix requested a review from miovd January 23, 2023 09:42
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
@Test
void igmsDestructiveMerge() throws IOException {

FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Copy link
Contributor

Choose a reason for hiding this comment

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

You can put this initialization in a setUp method annotated @Before

@Test
void igmsMergeWithMergingView() throws IOException {

FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark

CgmesConformity1Catalog.microGridBaseCaseBoundaries());
Network networkBENL = Network.read(mergedResourcesBENL.dataSource());

FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark

validate(serializedMergedNetwork, branchIds, generatorsId, voltageLevelIds);
}

public void validate(Network n, Set<String> branchIds, Set<String> generatorsId, Set<String> voltageLevelIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be static and private

voltageLevelIds.forEach(v -> assertNotNull(n.getVoltageLevel(v)));
}

public void exportNetwork(Network network, Path outputDir, String baseName, Map<String, Network> validNetworks, Set<String> profilesToExport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be static and private

emf/pom.xml Outdated
<scope>test</scope>
</dependency>

<!-- Runtime dependencies -->
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move these dependencies in test scope rather than runtime

Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Copy link
Contributor

@miovd miovd left a comment

Choose a reason for hiding this comment

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

Some comments about technical changes!
Regarding the functional scope, I think this is good for a first version, we can merge it for it to be in the next release and start a new PR improving it (particularly checking xnode and tielines).

private static FileSystem fs;

@BeforeAll
static void setUp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can be public and not static (fs should not be static)

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact BeforeAll force the setUp function to be static void, thus filesystem too. Should be fixed by moving to BeforeEach, not really necessary IMHO but could avoid read/write errors.

emf/src/test/java/com/powsybl/emf/IGMmergeTests.java Outdated Show resolved Hide resolved
Set<String> generatorsId = new HashSet<>();
Set<String> voltageLevelIds = new HashSet<>();

networkBENL.getBranches().forEach(b -> branchIds.add(b.getId()));
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent the encoding error (it will not be added in this release), do

networkBENL.getBranches().forEach(b -> branchIds.add(b.getId().replace(" ", "%20)); // FIXME workaround before fixing CGMES export/import

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.

Comment on lines 202 to 236
private static void exportEquipment(Network network, CgmesExportContext context, Path file) {
try (OutputStream out = Files.newOutputStream(file)) {
XMLStreamWriter writer = XmlUtil.initializeWriter(true, " ", out);
EquipmentExport.write(network, writer, context);
} catch (IOException | XMLStreamException e) {
throw new RuntimeException(e);
}
}

private static void exportTopology(Network network, CgmesExportContext context, Path file) {
try (OutputStream out = Files.newOutputStream(file)) {
XMLStreamWriter writer = XmlUtil.initializeWriter(true, " ", out);
TopologyExport.write(network, writer, context);
} catch (IOException | XMLStreamException e) {
throw new RuntimeException(e);
}
}

private static void exportSteadyStateHypothesis(Network network, CgmesExportContext context, Path file) {
try (OutputStream out = Files.newOutputStream(file)) {
XMLStreamWriter writer = XmlUtil.initializeWriter(true, " ", out);
SteadyStateHypothesisExport.write(network, writer, context);
} catch (XMLStreamException | IOException e) {
throw new RuntimeException(e);
}
}

private static void exportStateVariable(Network network, CgmesExportContext context, Path file) {
try (OutputStream out = Files.newOutputStream(file)) {
XMLStreamWriter writer = XmlUtil.initializeWriter(true, " ", out);
StateVariablesExport.write(network, writer, context);
} catch (IOException | XMLStreamException e) {
throw new RuntimeException(e);
}
}
Copy link
Contributor

@miovd miovd Jan 27, 2023

Choose a reason for hiding this comment

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

Those methods can be refactored by adding a Runnable in parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it with a consumer functionnal interface.

obrix added 2 commits January 30, 2023 10:42
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>

import static org.junit.jupiter.api.Assertions.assertNotNull;

class IGMmergeTests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add your name as @author in the javadoc?

* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the SPDX?

@@ -0,0 +1,218 @@
/*
* Copyright (c) 2022, RTE (http://www.rte-france.com)
Copy link
Contributor

Choose a reason for hiding this comment

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

*2023

obrix added 2 commits January 30, 2023 11:55
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@miovd miovd changed the title [WIP] EMF module and merge tests EMF module and merge tests Jan 30, 2023
@miovd miovd merged commit b74ac7c into main Jan 31, 2023
@miovd miovd deleted the emf-integration branch January 31, 2023 14:55
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.

2 participants