[BugFix] fast_any_tap boundary handling boundary handling in binary search

[Jerry-Jinfeng-Guo]

Fixes #1215

The behaviour that lead to the original bug was due to us strictly limiting binary search to search between the upper bound and lower bounds. In cases where target voltage range lives outside such bounds, boundary taps should be considered.

Some other tests are also updated. Due to the nature of including a boundary, binary search will terminate at lower_bound == upper_bound and will not run all the way until max_iter reached. This will change the behaviour of the search wrt maximum iteration exceptions.

Arguably, if target range falls outside the achievable range, we return the closest tap without exception, the same should apply to small u_band.

[Jerry-Jinfeng-Guo]

Pure binary search runs will not trigger max iteration error.

[mgovers]

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]

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

[Jerry-Jinfeng-Guo]

Discussion needed wrt behaviour.

[mgovers]

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
  • This is the bug talked about in [BUG] fast_any_tap incorrectly handles voltage bands outside the achievable voltage range #1215
• 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]

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]

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]

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.

[figueroa1395]

Only small remarks made

[figueroa1395]

Side note: std::midpoint(10, 21) == 15 and std::midpoint(21, 10) == 16, so the description is wrong. It doesn't affect the current bug-fix, but nice to address it at once.

[Jerry-Jinfeng-Guo]

See 80eea98

[Jerry-Jinfeng-Guo]

and b15c322

[figueroa1395]

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?

[figueroa1395]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud