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

Delete Merging View implementation #2612

Merged
merged 5 commits into from
Jul 3, 2023
Merged

Delete Merging View implementation #2612

merged 5 commits into from
Jul 3, 2023

Conversation

annetill
Copy link
Member

@annetill annetill commented Jun 13, 2023

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?

Maybe issues are related to the merging view (see issues). After the tie lines refactoring, the sub-networks modeling will be the only tool box for CGMES profiles export.

What kind of change does this PR introduce?

The merging view as IIDM implementation is no more available, too risky to use too hard and costly to fix directly.

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?

  • 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:

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@mathbagu
Copy link
Contributor

Don't forget to remove references to the merging view in the website. A quick search gives me the list of artifacts, but there are potential other pages.

annetill added 2 commits June 14, 2023 07:45
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@@ -114,22 +113,19 @@ void microGridBaseCaseBEMergingViewNL() {
// Both networks have the same number of dangling lines
assertEquals(nDlBE, nDlNL);

Network n = MergingView.create("be-nl", "CGMES");
n.merge(nl, be);
be.merge(nl);
Copy link
Member Author

Choose a reason for hiding this comment

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

We see here that we have an equivalence now between the merging view and the destructive merge for equipments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test can be merged with the test above microGridBaseCaseBEMergedWithNL, they check the same thing now...

compareMerges(tieLineId, assembled, ds1, ds2);
}

private static void compareMerges(String tieLineId, ReadOnlyDataSource dsAssembled, ReadOnlyDataSource ds1, ReadOnlyDataSource ds2) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tie lines in the merging view and in the destructive merge are now equivalent. These tests can be deleted.

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.

Mostly okay for me, one small comment.
Before deleting the merging view, be sure to check that the EMF process is equivalent with sub-networks and the merging view :)

@@ -114,22 +113,19 @@ void microGridBaseCaseBEMergingViewNL() {
// Both networks have the same number of dangling lines
assertEquals(nDlBE, nDlNL);

Network n = MergingView.create("be-nl", "CGMES");
n.merge(nl, be);
be.merge(nl);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test can be merged with the test above microGridBaseCaseBEMergedWithNL, they check the same thing now...

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@annetill annetill changed the title [WIP] Delete Merging View Delete Merging View Jun 21, 2023
@annetill annetill added the Breaking Change API is broken label Jun 21, 2023
@annetill annetill changed the title Delete Merging View Delete Merging View implementation Jun 21, 2023
@annetill annetill requested review from geofjamg and removed request for mathbagu and flo-dup June 21, 2023 10:07
@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

@olperr1
Copy link
Member

olperr1 commented Jun 30, 2023

Don't forget to remove references to the merging view in the website. A quick search gives me the list of artifacts, but there are potential other pages.

The merging view is also used in the "powsybl/powsybl-tutorials" repository.

@olperr1 olperr1 merged commit 3dba632 into main Jul 3, 2023
@olperr1 olperr1 deleted the delete-merging-view branch July 3, 2023 06:30
FranckLecuyer pushed a commit that referenced this pull request Jul 4, 2023
* Delete merging view iidm implementation.
* Clean CGMES tests.

---------

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change API is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants