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.

Sign up for GitHub

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

Jump to bottom

Newton Raphson State Esimation Implementation #438

Merged
merged 144 commits into from Feb 8, 2024
Merged

Newton Raphson State Esimation Implementation #438

merged 144 commits into from Feb 8, 2024

Conversation

nitbharambe
Copy link
Member

@nitbharambe nitbharambe commented Nov 27, 2023 •
edited

Relates to #369

The tests distribution-case and sensor-update are marked as xfail.
The fails are minor differences and pattern is difficult to find. Needs deeper dive.

@mgovers mgovers mentioned this pull request Nov 29, 2023
Merged
@mgovers mgovers changed the base branch from main to feature/NRSE-interface November 29, 2023 12:19
Base automatically changed from feature/NRSE-interface to main November 29, 2023 13:23
@nitbharambe nitbharambe changed the title Add Newton Raphson State Esimation structure Newton Raphson State Esimation Implementation Dec 20, 2023
@nitbharambe
Revert "remove result"
299943c 
This reverts commit 1b3e65b.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "remove space"
a7439c8 
This reverts commit 8d7bdea.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "remove unused variable function"
7c42656 
This reverts commit 7e17112.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "remove unused variable"
4c2b741 
This reverts commit 20ccead.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "remove extra line"
e1e9c23 
This reverts commit 855c3bb.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "remove all content"
7758fbf 
This reverts commit 905c221.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
Revert "nest namespace"
0622faf 
This reverts commit 39581d1.

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe nitbharambe changed the base branch from main to feature/NRSE-low-level-design-2 December 20, 2023 13:05
@nitbharambe
Merge branch 'feature/NRSE-low-level-design-2' into feature/NRSE-gene…
2d1d937 
…ral-structure

Signed-off-by: Nitish Bharambe <78108900+nitbharambe@users.noreply.github.com>
Base automatically changed from feature/NRSE-low-level-design-2 to main December 22, 2023 10:28
nitbharambe and others added 9 commits December 22, 2023 11:42
@nitbharambe
Merge branch 'main' into feature/NRSE-general-structure
ed88dcf 
Signed-off-by: Nitish Bharambe <78108900+nitbharambe@users.noreply.github.com>
@nitbharambe
address peters comments
bdd7ccb 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
[skip ci] rename attribute
c6601f6 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
[skip ci] new branch shunt wip
a927b66 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
[skip ci] injection wip
08cdbad 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
[skip ci] uncommenting left
d993d8e 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
clean up new workout
cc31b21 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
minor fix
a8c30bb 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe
comment unused
e0868a2 
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
mgovers and others added 3 commits February 6, 2024 11:11
@mgovers
fix bug in measured values
f7e2cf0 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
fix clang tidy
cdcd535 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Merge pull request #478 from PowerGridModel/fix/nrse-line-measurements
5eddc40 
Fix NRSE line measurement implementation
@TonyXiang8787
Copy link
Member

@mgovers are validation tests passing? According to @nitbharambe there are still failures?

@mgovers
back to any zero for measured values
b150f1c 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member

mgovers commented Feb 6, 2024

@mgovers are validation tests passing? According to @nitbharambe there are still failures?

There are 2 validation cases still failing for which @nitbharambe claims are likely due to different convergence points. In both cases, the algorithm converges and some residuals are less while others are more, hence his conclusion.

@nitbharambe , @petersalemink95 and I agreed on the following approach:

  1. Only do code review on existing functionality + code quality on this branch
  2. Merge this branch - release the NRSE as an experimental beta feature. This can be communicated to the stakeholders as such
  3. Follow-up PRs to add 2 separate validation cases on the currently failing ones that have the new saturation point as expected output
  4. Mathematical review on main after merge.
  5. After all these steps are done, the experimental flag can be removed and the full feature can be released as v1.7.0

@mgovers
remove todo
f9d43c2 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers mentioned this pull request Feb 7, 2024
Merged
Jerry-Jinfeng-Guo
Jerry-Jinfeng-Guo reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

My two cents on the code only.

...model_c/power_grid_model/include/power_grid_model/math_solver/iterative_linear_se_solver.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/measured_values.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/measured_values.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
@mgovers
resolve comments
34de38e 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Jerry-Jinfeng-Guo
Jerry-Jinfeng-Guo reviewed Feb 8, 2024
View reviewed changes
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

Reviews on the nr_se_solver document.

...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Outdated Show resolved Hide resolved
...d_model_c/power_grid_model/include/power_grid_model/math_solver/newton_raphson_se_solver.hpp Show resolved Hide resolved
@mgovers
resolve comments
284e6d4 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Jerry-Jinfeng-Guo
Jerry-Jinfeng-Guo previously approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

I have no more complaints, please approve my approval.

mgovers
mgovers previously approved these changes Feb 8, 2024
View reviewed changes
@mgovers
fix code smells
a6ac10d 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers dismissed stale reviews from Jerry-Jinfeng-Guo and themself via a6ac10d February 8, 2024 14:46
mgovers
mgovers previously approved these changes Feb 8, 2024
View reviewed changes
@mgovers
fix code smells
05c8b68 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
mgovers
mgovers previously approved these changes Feb 8, 2024
View reviewed changes
@mgovers
fix typo
9d4751c 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 8, 2024

Quality Gate Passed Quality Gate passed

Issues
7 New issues

Measures
0 Security Hotspots
98.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mgovers mgovers merged commit e7ea632 into main Feb 8, 2024
27 checks passed
@mgovers mgovers deleted the feature/NRSE-general-structure branch February 8, 2024 15:35
@mgovers mgovers mentioned this pull request Feb 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TonyXiang8787 TonyXiang8787 TonyXiang8787 left review comments

@petersalemink95 petersalemink95 petersalemink95 left review comments

@mgovers mgovers mgovers approved these changes

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nitbharambe @TonyXiang8787 @mgovers @Jerry-Jinfeng-Guo @petersalemink95