Skip to content

[BugFix] fast_any_tap boundary handling boundary handling in binary search #1632

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

Open
Jerry-Jinfeng-Guo wants to merge 3 commits into
base: main
Choose a base branch
from
+14 −18
Open

[BugFix] fast_any_tap boundary handling boundary handling in binary search #1632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67be1cb
added boundary handling in binary search
Jerry-Jinfeng-Guo Oct 8, 2026
80eea98
address comment on outdated in code comment
Jerry-Jinfeng-Guo Oct 17, 2026
b15c322
minor adition
Jerry-Jinfeng-Guo Oct 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions ...id_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,13 +893,21 @@
IntS search(bool prefer_higher_) const {
// This logic is used to determine which of the middle points could be of interest
// given strategy used in optimization:
// Since in BinarySearch we insist on absolute upper and lower bounds, we only need to
// find the corresponding mid point. std::midpoint returns always the lower mid point
// In BinarySearch we rely on upper and lower bounds to find the corresponding mid point.
// std::midpoint returns always the relative 'lower' mid point
// if the range is of even length. This is why we need to adjust bounds accordingly.
// Not because upper bound and lower bound might be reversed, which is not possible.
// In the case the optimization target is beyond the range, both boundaries should be
// considered, especially in the final iteration of the binary search.
bool const prefer_higher = control_at_tap_side_ != prefer_higher_;
auto const primary_bound = prefer_higher ? upper_bound_ : lower_bound_;
auto const secondary_bound = prefer_higher ? lower_bound_ : upper_bound_;
auto const current_at_bound = current_ == primary_bound || current_ == secondary_bound;
bool const is_one_apart = std::abs(static_cast<int>(primary_bound) - secondary_bound) == 1;
// Handle edge case: 1 off and current already not satisfying
if (is_one_apart && current_at_bound) {

Check warning on line 908 in power_grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use the init-statement to declare "is_one_apart" inside the if statement. 


See more on https://sonarcloud.io/project/issues?id=PowerGridModel_power-grid-model&issues=AaEbM7GVRaD1HFdwPtiE&open=AaEbM7GVRaD1HFdwPtiE&pullRequest=1632
return current_ == primary_bound ? secondary_bound : primary_bound;
}
return std::midpoint(primary_bound, secondary_bound);
}

Expand Down
4 changes: 0 additions & 4 deletions ...ata/power_flow/automatic-tap-regulator/achievable-voltage-above-band-fast-any/params.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,5 @@
"atol": {
"default": 1e-5,
"u": 100.0
},
"xfail": {
"raises": "MaxIterationReached",
"reason": "Bug in fast any tap changing strategy in this edge case: https://github.com/PowerGridModel/power-grid-model/issues/1215"
}
}
6 changes: 1 addition & 5 deletions .../power_flow/automatic-tap-regulator/auto-tap-changer-meshed-fast-any-max-iter/params.json
View file
Open in desktop

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pure binary search runs will not trigger max iteration error.

@mgovers mgovers Oct 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so what does it return if there are 2 conflicting tap changers? Does it choose either of the settings? How does it choose which one is better?

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The built-in reset mechanism will handle that and the top layer optimization logic remains the same.

@figueroa1395 figueroa1395 Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name of the folder suggests that this case would reach max-iterations, right? Can we change it so we don't get confused? Same for below.

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we do intend to revisit the any strategy to unify the behavior, I would do the renaming in that follow-up. I will put some comment in the test so that this is clear.

Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,5 @@
"calculation_method": "newton_raphson",
"tap_changing_strategy": "fast_any_tap",
"rtol": 1e-5,
"atol": 1e-5,
"raises": {
"raises": "MaxIterationReached",
"reason": "TapPositionOptimizer::iterate"
}
"atol": 1e-5
}
6 changes: 1 addition & 5 deletions ..._flow/automatic-tap-regulator/auto-tap-changer-small-u-band-max-iter-fast-any/params.json
View file
Open in desktop

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Discussion needed wrt behaviour.

@mgovers mgovers Oct 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We originally talked about 2 edge cases in which the boundary handling needs an explicit choice:

  • u_set + u_band too low for all tap positions, or u_set - u_band too high for all tap positions
  • small u_band: some tap positions result in a voltage below u_set - u_band and some tap positions result in a voltage above u_band + u_band (this case)
    • This case is ambiguous and we indeed should discuss now whether to hard error or allow it for binary search.
    • I believe that binary search setting on one or the other makes sense (I agree with your choice)
    • How does this work for max and min strategies, respectively? Are these edge cases handled correctly now?

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, we should have this discussion. As for min and max strategies, they are both 'unaffected' thanks to the explore_neighbourhood we put in place for the final wiggle.

@figueroa1395 figueroa1395 Oct 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm unresolving because I have a question that's likely related: The same test case for any strategy still fails with MaxIterationReached, shouldn't both behave the same in this case, or at least both have the same behaviour even if the final tap position isn't the same?

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Oct 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am in principle agreeing with your point. However, I would argue that we need to align the behavior of any strategy to that of the physical behavior. That is, if a ping-poinging behavior is detected during an any search, the search should terminate. This will eliminate the infinite loops we may have with ultra small u_band and return a tap pos that is close to said band.

In short, we should revisit the any strategy.

Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,5 @@
"calculation_method": "newton_raphson",
"tap_changing_strategy": "fast_any_tap",
"rtol": 1e-5,
"atol": 1e-5,
"raises": {
"raises": "MaxIterationReached",
"reason": "TapPositionOptimizer::binary_search"
}
"atol": 1e-5
}
4 changes: 2 additions & 2 deletions ...w/automatic-tap-regulator/auto-tap-changer-small-u-band-max-iter-fast-any/sym_output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
"data": {
"node": [
{"id": 1, "u_pu": 1, "u": 9999.9},
{"id": 2, "u_pu": 1.02456, "u": 409.8260092982474}
{"id": 2, "u_pu": 0.9989, "u": 399.5626}
],
"transformer_tap_regulator": [
{"id": 6, "tap_pos": 2}
{"id": 6, "tap_pos": 3}
]
}
}
Loading