Skip to content

Conversation

@figueroa1395
Copy link
Member

This is an experimentation PR aimed to remove all the stuff related to preparing the calculation/solvers out of the main model. This PR should be an enabler to unit test the touched logic more easily, keep trimming the main model, and get it ready to remove everything related to the calculation logic in a followup.

At this stage I'm gathering input. I will address and update accordingly.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 self-assigned this Oct 24, 2025
@figueroa1395 figueroa1395 added improvement Improvement on internal implementation do-not-merge This should not be merged labels Oct 24, 2025
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a global review, not an in-depth one. Awesome cleanup!

: system_frequency_{system_frequency},
meta_data_{&input_data.meta_data()},
math_solver_dispatcher_{&math_solver_dispatcher} {
solver_preparation_context_{.math_solver_dispatcher = &math_solver_dispatcher} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction can IMO go to MainModel instead of MainModelImpl just like JobDispatcher

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this in 66e5449. There are now some additional dependencies to prepare_calculate.hpp which I'm not a fan of, but I suppose that should be fine because it's only temporal until we really get rid of MainModelImpl. Thoughts?

namespace power_grid_model {
struct SolverPreparationContext {
main_core::MathState math_state;
Idx n_math_solvers{0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required to keep n_math_solvers?
Is it ever not equivalent to math_state.topology.size()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it (in 4f0d589) in favor of a helper function that does the calling from state.math_topology.size() indeed. It should be fine and

assert(n_math_solvers == static_cast<Idx>(main_core::get_y_bus<sym>(solver_context.math_state).size()));|

still remains as a sanity check.

Let me know what you think.

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge This should not be merged improvement Improvement on internal implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants