Skip to content

Conversation

@Jerry-Jinfeng-Guo
Copy link
Member

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo commented Oct 24, 2025

The binary search logic in any mode can not handle edge case when u_band is significantly smaller than tap_size due to the lack of back-forth jumping behavior. This PR fixes that and fast_any_tap strategy now correctly throws a MaxIterationReached error in equivalent cases as other strategies do.

  • The fix
  • Tests

Note: the edge case is in itself not a valid input, as unrealistic u_band shouldn't be allowed, this PR aligns the behavior across different strategies.

This PR relates to #887

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo self-assigned this Oct 24, 2025
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the bug Something isn't working label Oct 24, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the fast_any_tap strategy where the binary search logic fails to handle edge cases when u_band is significantly smaller than tap_size. The fix ensures that fast_any_tap now correctly throws a MaxIterationReached error in cases equivalent to other strategies, aligning behavior across different tap-changing strategies.

Key changes:

  • Modified binary search logic to detect when no valid tap position exists within the search range
  • Added validation to throw MaxIterationReached when the search cannot converge
  • Added test cases for both fast_any_tap and any_valid_tap strategies with extremely small u_band values

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

File Description
power_grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp Adds rewind method and validation logic to detect invalid tap positions in binary search
tests/data/power_flow/automatic-tap-regulator/auto-tap-changer-repro-fast-any/* Test case for fast_any_tap strategy with unrealistically small u_band (0.00001) expecting MaxIterationReached
tests/data/power_flow/automatic-tap-regulator/auto-tap-changer-repro-any/* Test case for any_valid_tap strategy with same configuration for comparison

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
…-meshed-any-max-iter/params.json

Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
@sonarqubecloud
Copy link

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the do-not-merge This should not be merged label Oct 29, 2025
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo changed the title BugFix: fast_any_tap can not handle small u_band (smaller than tap_size) PotentialBugFix: fast_any_tap can not handle small u_band (smaller than tap_size) Oct 29, 2025
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

apparently, i still had some comments on Pending 😕 Apologies for only now submitting the review

}

if (strategy_ == OptimizerStrategy::fast_any) {
if (strategy_ == OptimizerStrategy::fast_any && cmp == 0) { // NOLINT(modernize-use-nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

I know there is a false positive regarding cmp, but please use ==equivalent rather than ==0` if possible. I think it's something along the lines of

Suggested change
if (strategy_ == OptimizerStrategy::fast_any && cmp == 0) { // NOLINT(modernize-use-nullptr)
if (strategy_ == OptimizerStrategy::fast_any && cmp == std::partial_ordering::equivalent) {

// _tap_changed_ is an aggregated flag with _inevitable_run_, so we can not use it here
if (tap_pos == transformer.tap_pos() && cmp != 0 && !current_bs.get_end_of_bs()) {
current_bs.rewind(tap_pos, transformer.tap_min(), transformer.tap_max());
throw MaxIterationReached{std::format(
Copy link
Member

Choose a reason for hiding this comment

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

I still am not 100% convinced that this really is undesirable behavior: just like fast_int in gcc: it works in cases it's designed to work in, but if not, the behavior is still well-behaved, and it is opt-in (you chose to go with fast_any in the first place).

if it couldn't find a solution, it should return either the lower or the upper bound that was found, but not MaxIterationReached because it did not exceed the amount of iterations. If you really want to raise an error, then maybe NoValidTapFound or something

Also, in real-world cases where the intention is not to get the exact state but to get an approximate one, it's undesirable to throw a hard error. Users may in fact be OK with an approximate solution

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

Labels

bug Something isn't working do-not-merge This should not be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants