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

Adder by copy for lines #3208

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

Adder by copy for lines #3208

wants to merge 11 commits into from

Conversation

nao1345678
Copy link
Contributor

@nao1345678 nao1345678 commented Nov 13, 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)

What kind of change does this PR introduce?

It allows the lines to be initialized from another already existing one.

What is the current behavior?

Lines are not allowed to be duplicated.

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)

Custom IIDM implementation maintainers should add the following method in their Network implementations:

  • LineAdder newLine(Line line).

Other information:

Signed-off-by: Naledi EL CHEIKH <[email protected]>
@olperr1 olperr1 added Breaking Change API is broken Custom IIDM impl and removed Breaking Change API is broken labels Dec 16, 2024
Comment on lines 368 to 375
return newLine(null, line).setR(line.getR())
.setX(line.getX())
.setG1(line.getG1())
.setG2(line.getG2())
.setB1(line.getB1())
.setB2(line.getB2())
.setVoltageLevel1(line.getTerminal1().getVoltageLevel().getId())
.setVoltageLevel2(line.getTerminal2().getVoltageLevel().getId());
Copy link
Member

Choose a reason for hiding this comment

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

The setters should be called in the adder's constructor (or in a utility method, but called in the constructor).
In the current state, these variables are not set when newLine(Line) is called from a subnetwork.

Copy link
Member

Choose a reason for hiding this comment

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

BranchUtil is in the iidm-impl module here. I think it would be better to have this method in the iidm-api module, so it could be reused by custom IIDM implementations (such as powsybl-network-store).

I think you can define it as a static method directly in LineAdder.

Another think. It would be better to call it in LineAdderImpl(NetworkImpl network, String subnetwork, Line copiedLine). It would be counterintuitive that this method initializes the limits, but not the line attributes.

@nao1345678 nao1345678 requested a review from olperr1 December 19, 2024 13:54
@olperr1
Copy link
Member

olperr1 commented Dec 19, 2024

Could you also add a test with a line in a subnetwork?
You can take inspiration from "AbstractSubnetworksCreationTest` to create a network having a subnetwork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants