Main model refactoring: move calculation logic out of main model#1286
Main model refactoring: move calculation logic out of main model#1286
Conversation
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
…ut-of-main-model Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
mgovers
added
improvement
…atforms Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
figueroa1395
left a comment
figueroa1395
left a comment
There was a problem hiding this comment.
Current state looks good, I left some minor comments only.
On the other hand, I think calculate and the calculate_ overloads should be gone from the main model as well, and we should only leave the calculator entry point, the others are implementation details that can be in a namespace detail in calculation.hpp. What do you think? Any specific reason to not take them out?
| constexpr auto enumerate(std::ranges::viewable_range auto&& rng) { | ||
| return std::views::zip(IdxRange{std::ssize(rng)}, rng); |
There was a problem hiding this comment.
rng can be easily miss-interpreted. Can you just use range instead?
| constexpr auto enumerate(std::ranges::viewable_range auto&& rng) { | |
| return std::views::zip(IdxRange{std::ssize(rng)}, rng); | |
| constexpr auto enumerate(std::ranges::viewable_range auto&& range) { | |
| return std::views::zip(IdxRange{std::ssize(range)}, range); |
| std::same_as<std::invoke_result_t<SolveFn, MathSolverType&, YBus const&, InputType const&>, | ||
| SolverOutputType> | ||
| std::vector<SolverOutputType> calculate_(PrepareInputFn&& prepare_input, SolveFn&& solve, Logger& logger) { | ||
| std::ranges::range<std::invoke_result_t<PrepareInputFn, Idx /*n_math_solvers*/>> && |
There was a problem hiding this comment.
Were the changes from std::same_as<a, std::vector<b>> to std::ranges::range<a, b> just so it's more general and up to date code? Do you expect to have potential std::spans for instance at some point? Wouldn't it be best to keep it restricted to std::vector` only if we don't foresee this?
There was a problem hiding this comment.
i needed access to the more generic type, otherwise it was not able to infer the type
| return [this, &comp_coup = state_.comp_coup, &options, &logger](MainModelState const& state, | ||
| CalculationMethod calculation_method) { | ||
| (void)state; // to avoid unused-lambda-capture when in Release build | ||
| assert(&state == &state_); | ||
|
|
||
| return calculate_<MathSolverProxy<sym>, YBus<sym>>(Calc::preparer(state, comp_coup, options), | ||
| Calc::solver(calculation_method, options, logger), | ||
| logger); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Why not try to get rid of the state and state_ "shenanigans" and do something like (beware of typos)
| return [this, &comp_coup = state_.comp_coup, &options, &logger](MainModelState const& state, | |
| CalculationMethod calculation_method) { | |
| (void)state; // to avoid unused-lambda-capture when in Release build | |
| assert(&state == &state_); | |
| return calculate_<MathSolverProxy<sym>, YBus<sym>>(Calc::preparer(state, comp_coup, options), | |
| Calc::solver(calculation_method, options, logger), | |
| logger); | |
| }; | |
| }; | |
| return [this, &options, &logger](MainModelState const& state, | |
| CalculationMethod calculation_method) { | |
| return calculate_<MathSolverProxy<sym>, YBus<sym>>(Calc::preparer(state, state.comp_coup, options), | |
| Calc::solver(calculation_method, options, logger), | |
| logger); | |
| }; | |
| }; |
There was a problem hiding this comment.
mutable vs immutable. the input state is presumed const, but the comp coup needs to be mutable
| calculation_symmetry, std::forward<Functor>(f), std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| template <typename T, typename sym> struct Calculator; |
power_grid_model_c/power_grid_model/include/power_grid_model/calculation.hpp
Show resolved
Hide resolved
…alculation.hpp Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com> Co-authored-by: Nitish Bharambe <78108900+nitbharambe@users.noreply.github.com> Signed-off-by: Martijn Govers <martygovers@hotmail.com>
|
4 participants
This finishes up the cleanup of main model