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

Refactor DC Woodbury #1111

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Refactor DC Woodbury #1111

merged 4 commits into from
Nov 12, 2024

Conversation

SylvestreSakti
Copy link
Contributor

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?
No

What kind of change does this PR introduce?
This PR follows this PR: #1109
Some variable names and comments have been modified to ensure a better maintanability of the code

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

Signed-off-by: PRABAKARAN Sylvestre <sylvestre.prabakaran@rte-france.com>
@vidaldid-rte
Copy link
Collaborator

vidaldid-rte commented Nov 5, 2024

There are still type of contingenies in DcSensitivyAnalysis where a new DenseMatrix is created (For example in caculateFactorStates. This can lead to the same memory issues in cases where many contingency fall in such category.

It would be good to have a resizableDenseMatrix that can grow its directBuffer if needed when resized, and could be used as a working matrix for all contingencies.
In the same idea, for WoodburyDCSecurityAnalysis, we could have a large reusable array of double that could grow when needed.

But this is maybe out of the scope of this PR since it needs an evolution i core.

Signed-off-by: PRABAKARAN Sylvestre <sylvestre.prabakaran@rte-france.com>
@SylvestreSakti
Copy link
Contributor Author

There are still type of contingenies in DcSensitivyAnalysis where a new DenseMatrix is created (For example in caculateFactorStates. This can lead to the same memory issues in cases where many contingency fall in such category.

It would be good to have a resizableDenseMatrix that can grow its directBuffer if needed when resized, and could be used as a working matrix for all contingencies. In the same idea, for WoodburyDCSecurityAnalysis, we could have a large reusable array of double that could grow when needed.

But this is maybe out of the scope of this PR since it needs an evolution i core.

Yes, I agree with that, a "ResizableDenseMatrix" would help using less memory. But indeed may be out of this scope (and would need a modification in powsybl-core)

Comment on lines 206 to 207
// create workingContingencyStates that will be a working copy of pre-contingency states
double[] workingContingencyStates = preContingencyStates.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Use System.arraycopy instead of clone ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ! I did not know about this subtle difference but seems slightly better indeed (https://stackoverflow.com/questions/7179251/is-there-any-reason-to-prefer-system-arraycopy-over-clone)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was true for Java5 in 2006 but if you run the bench today you see no difference after the JIT has comleted.

Signed-off-by: PRABAKARAN Sylvestre <sylvestre.prabakaran@rte-france.com>
@geofjamg geofjamg merged commit 6650b47 into main Nov 12, 2024
7 checks passed
@geofjamg geofjamg deleted the refacto_for_dc_woodbury branch November 12, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants