Optimization: the meshed observability check#1301
Optimization: the meshed observability check#1301Jerry-Jinfeng-Guo wants to merge 19 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>
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>
|
| constexpr auto min_avg_degree = 2; | ||
| Idx const avg_degree = | ||
| (avg_degree_size_t >= min_avg_degree) ? static_cast<Idx>(avg_degree_size_t) : static_cast<Idx>(min_avg_degree); |
There was a problem hiding this comment.
Can you move this to below where it gets used in max_iterations caluclation?
There was a problem hiding this comment.
Also,
It would be direct if we dont move to the degree area. I see n_bus is divided and multiplied. So
n_visits = max(total_edges, min_avg_degree * n_bus)
Then below
max_iter_unclamped = n_visits * n_stages
(n_visits might not be the best name)
| auto const max_iterations = | ||
| static_cast<Idx>(std::min(max_iter_unclamped, static_cast<std::size_t>(std::numeric_limits<Idx>::max()))); |
There was a problem hiding this comment.
Why this extra check on numeric limits max?
|
2 minor comments on naming/refactoring. LGTM otherwise. @Jerry-Jinfeng-Guo |
4 participants
There are several places for improvement from #1136. This PR follows up on that.