BugFix: unify fast_any_tap behavior when handling small u_band (smaller than tap_size)
#1169
Conversation
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
There was a problem hiding this comment.
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
MaxIterationReachedwhen the search cannot converge - Added test cases for both
fast_any_tapandany_valid_tapstrategies with extremely smallu_bandvalues
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.
..._grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
tests/data/power_flow/automatic-tap-regulator/auto-tap-changer-meshed-any-max-iter/params.json
Outdated
Show resolved
Hide resolved
…-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>
|
Jerry-Jinfeng-Guo
changed the title
fast_any_tap can not handle small u_band (smaller than tap_size)fast_any_tap can not handle small u_band (smaller than tap_size)
mgovers
left a comment
mgovers
left a comment
There was a problem hiding this comment.
apparently, i still had some comments on Pending 😕 Apologies for only now submitting the review
..._grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
Outdated
Show resolved
Hide resolved
..._grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
Show resolved
Hide resolved
...data/power_flow/automatic-tap-regulator/auto-tap-changer-meshed-fast-any-max-iter/input.json
Show resolved
Hide resolved
|
Please rename the PR to follow the PR naming guidelines |
Jerry-Jinfeng-Guo
changed the title
fast_any_tap can not handle small u_band (smaller than tap_size)fast_any_tap behavior when handling small u_band (smaller than tap_size)
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
..._grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp
Show resolved
Hide resolved
|
for the readthedocs build, you're probably missing a |
|
|
||
| auto constexpr operator<=>(TrafoGraphEdge const& other) const { | ||
| if (auto cmp = weight <=> other.weight; cmp != 0) { // NOLINT(modernize-use-nullptr) | ||
| if (auto cmp = weight <=> other.weight; !std::is_eq(cmp)) { |
There was a problem hiding this comment.
Is there a difference between !std::is_eq and std:is_neq? If not, minor nitpick to use std:is_neq instead :)
|
|
I deem most of the comments are complimentary and or out of scope. Please see whether a final review is possible? @mgovers @figueroa1395 |
The binary search logic in any mode can not handle edge case when
u_bandis significantly smaller thantap_sizedue to the lack of back-forth jumping behavior. This PR fixes that andfast_any_tapstrategy now correctly throws aMaxIterationReachederror in equivalent cases as other strategies do.Note: the edge case is in itself not a valid input, as unrealistic
u_bandshouldn't be allowed, this PR aligns the behavior across different strategies.This PR relates to #887