Optimization: the meshed observability check#1301
Optimization: the meshed observability check#1301Jerry-Jinfeng-Guo wants to merge 18 commits intomainfrom
Conversation
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
There was a problem hiding this comment.
Pull request overview
Follow-up optimization of the “meshed observability check” spanning-tree search to reduce overhead per iteration and cap runtime more tightly.
Changes:
- Replaced repeated
std::ranges::count(...)with an O(1)visited_counttracker. - Introduced a degree-based
max_iterationslimit and added small allocation optimizations (reserve). - Removed a previously maintained “discovered edges” vector used only for documentation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.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>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
|
Apart from the three complains about |
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Show resolved
Hide resolved
| throw NotObservableError{"Meshed observability check fail. Network unobservable.\n"}; | ||
| // Failed attempt: rollback all modifications before trying next candidate | ||
| for (auto const& mod : modifications) { | ||
| *mod.ptr = mod.old_value; |
There was a problem hiding this comment.
Why not create a create a copy of modifications. And then reset it at this point?
What does the ptr achieve that this cant?
There was a problem hiding this comment.
probably also should be moved, not passed by reference, because otherwise you may run into lifetime issues
There was a problem hiding this comment.
This is one of the optimizations done. The copy adds to the footprint and drags down runtime.
| Idx const max_iterations = n_bus * n_bus; // prevent infinite loops | ||
| Idx iteration = 0; | ||
| // Context struct to hold shared state during spanning tree search | ||
| struct SpanningTreeContext { |
There was a problem hiding this comment.
Since we have to keep track of this state via pointers and then dereference it each time, why not enclose everthing in a class?
There was a problem hiding this comment.
Or alternatively, why not just declare and initialize this struct at find_spanning_tree_from_node directly, store the values there once (instead of references to the values) and pass this structure by reference/extract values by reference as needed.
I'm curious about why this choice, and why not other, besides my suggestion or Nitish's.
There was a problem hiding this comment.
See f37f7f2. As discussed offline: no strong preference.
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>
| .downwind = &downwind}; | ||
|
|
||
| // Iteration limit: visit all nodes plus some backtracking allowance | ||
| auto const max_iter_unclamped = static_cast<std::size_t>(n_bus) * static_cast<std::size_t>(avg_degree) * 3u; |
There was a problem hiding this comment.
Out of curiosity, why the choice of 3 here? Any special reason behind it? If so, can you make it a named variable with some meaning so we don't forget the intention later on?
There was a problem hiding this comment.
3 is a heuristic to account for the forward search, back tracking and different candidate retires
There was a problem hiding this comment.
i prefer a named variable over a comment, e.g.
constexpr auto n_stages = 3u; // heuristic: forward search, backtracking, candidate retries
auto const max_iter_unclamped = static_cast<std::size_t>(n_bus) * static_cast<std::size_t>(avg_degree) * n_stages;idem for the 2u hardcoded value elsewhere
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/observability.hpp
Show resolved
Hide resolved
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
|
4 participants
There are several places for improvement from #1136. This PR follows up on that.