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

[Feature] Automatic Tap Changer C API #597

Merged
merged 19 commits into from
May 21, 2024
Merged

[Feature] Automatic Tap Changer C API #597

merged 19 commits into from
May 21, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented May 14, 2024

  • Add automatic tap position as experimental feature to C API
  • Add PGM_TapChangingStrategy enum
  • Add PGM_set_tap_changing_strategy option setter

@mgovers
outer exposition for C API
0213172 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the feature New feature or request label May 14, 2024
mgovers and others added 5 commits May 15, 2024 09:17
@mgovers
Update power_grid_model_c/power_grid_model_c/include/power_grid_model…
ddec77d 
…_c/basics.h


Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

Signed-off-by: Martijn Govers <martygovers@hotmail.com>
@mgovers
prevent users from accidentally doing tap changing for state estimation
1a425ff 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
add experimental feature error
ff59cd3 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
make gcc happy
135a8d3 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
improve exception types for experimental feature
d582d92 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers mentioned this pull request May 15, 2024
Merged
@mgovers mgovers changed the base branch from main to feature/migrate-gcc-13 May 16, 2024 11:22
@mgovers mgovers changed the base branch from feature/migrate-gcc-13 to main May 16, 2024 11:23
mgovers and others added 6 commits May 17, 2024 10:09
@mgovers
Merge branch 'main' into feature/auto-tap-c-api
0f8b3aa 
Signed-off-by: Martijn Govers <martijn.govers@alliander.com>
@mgovers
fix CI
d8da83e 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
add optimization type + strategy to main model options
581fea3 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
add C API test that tap changing is experimental
8578bd6 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
add C API tests for experimental feature
684fb4e 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
actually create optimizer in main model
bb8bd68 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers marked this pull request as ready for review May 17, 2024 10:09
@mgovers
Apply suggestions from code review
2f3df3b 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
gcc has weird string behaviour (outputs 'l' instead of '__int64' or s…
47f5fde 
…imilar)

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
TonyXiang8787
TonyXiang8787 previously approved these changes May 18, 2024
View reviewed changes
@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue May 18, 2024
@TonyXiang8787 TonyXiang8787 removed this pull request from the merge queue due to a manual request May 18, 2024
@TonyXiang8787
Copy link
Member

@mgovers I have reviewed and approved the PR. I am not sure if this should now be merged before main_model is ready.

@mgovers
make sonar cloud happy
33f20f4 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Merge branch 'feature/auto-tap-c-api' of https://github.com/PowerGrid…
810aac1 
…Model/power-grid-model into feature/auto-tap-c-api

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
nitbharambe
nitbharambe previously approved these changes May 21, 2024
View reviewed changes
@mgovers
sonar cloud again
7b16edb 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Revert "sonar cloud again"
5a086b3 
This reverts commit 7b16edb.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
forwarding reference
ac23de1 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented May 21, 2024

@mgovers I have reviewed and approved the PR. I am not sure if this should now be merged before main_model is ready.

i don't think it matters. with the current implementation in this PR, you do get the optimal state - just not the optimal tap positions. I would consider it a missing feature rather than a critical feature.

After this PR is merged, the validation tests can already be created and even run - provided we do not check for the tap positions. We can also start the Python API creation.

Since we do not give any guarantees for experimental features, I think having missing features is OK.

If experimental_feature does not carry the load of feature_under_construction, then maybe we should add that flag as well. Maybe that's something up for discussion for our next big thing

@mgovers mgovers enabled auto-merge May 21, 2024 07:55
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 21, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@mgovers mgovers added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit d524afb May 21, 2024
26 checks passed
@mgovers mgovers deleted the feature/auto-tap-c-api branch May 21, 2024 09:21
@mgovers mgovers mentioned this pull request May 21, 2024
Merged
Jerry-Jinfeng-Guo
Jerry-Jinfeng-Guo reviewed May 21, 2024
View reviewed changes
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp
Comment on lines +98 to +100
OptimizerType optimizer_type{OptimizerType::no_optimization};
OptimizerStrategy optimizer_strategy{OptimizerStrategy::any};

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was under the impression this was intentionally left out of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

no that was #599 , which blocked work on the C API (this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@TonyXiang8787 TonyXiang8787 TonyXiang8787 approved these changes

@nitbharambe nitbharambe nitbharambe left review comments

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mgovers @TonyXiang8787 @Jerry-Jinfeng-Guo @nitbharambe