Cleanup main model: Clean up templates and variadics #1109

nitbharambe · 2025-09-02T12:37:25Z

Goals:

Clean MainModelImpl of duplicate/repeated templates.
Make MainModelImpl Testable for any collection of components.

Signed-off-by: Nitish Bharambe <[email protected]>

mgovers · 2025-09-02T12:49:49Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+template <typename Tuple> struct tuple_type_identities_to_tuple_types;
+
+template <typename... Ts> struct tuple_type_identities_to_tuple_types<std::tuple<std::type_identity<Ts>...>> {
+    using type = std::tuple<Ts...>;
+};


This is very cool!

mgovers · 2025-09-02T12:52:41Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/topology.hpp

+template <typename Tuple, std::size_t... Is>
+void register_all_components_impl(auto const& components, ComponentTopology& comp_topo, std::index_sequence<Is...>) {
+    (detail::register_topology_components<std::tuple_element_t<Is, Tuple>>(components, comp_topo), ...);
+}
+
+template <typename Tuple> void register_all_components(auto const& components, ComponentTopology& comp_topo) {
+    register_all_components_impl<Tuple>(components, comp_topo, std::make_index_sequence<std::tuple_size_v<Tuple>>{});
+}


nitbharambe · 2025-09-02T12:54:02Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/topology.hpp

+    (detail::register_topology_components<std::tuple_element_t<Is, Tuple>>(components, comp_topo), ...);
+}
+
+template <typename Tuple> void register_all_components(auto const& components, ComponentTopology& comp_topo) {


We can have a common run_functor_with_tuple too.

nitbharambe · 2025-09-02T12:58:36Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+template <class T, class U> struct MainModelType;
+
+template <class... ExtraRetrievableType, class... ComponentType>
+struct MainModelType<ExtraRetrievableTypes<ExtraRetrievableType...>, ComponentList<ComponentType...>> {


A struct which should contain all information about the MainModel being created.
The above utilities can be incorporated here as well.

One downside is the addition of typename at many places since this is a struct.

Also this is start of the experiment, it might be possible to clean up or come up with different ways altogether.

For future reference: We also have similar utility of get_cls_pos in container (Which is not container specific). They can be combined. (Also I read addressing via tuple is better than recursive)

One downside is the addition of typename at many places since this is a struct.

for most standard library objects, foo_t is a shorthand for typename foo::type. maybe that can clean up that part a little bit? I personally am not really concerned about the amount of typenames

Also I read addressing via tuple is better than recursive

100% 💯

I don't mind this at all, I actually find it very neat.

for most standard library objects, foo_t is a shorthand for typename foo::type. maybe that can clean up that part a little bit? I personally am not really concerned about the amount of typenames

I meant in the usage that we see in main_model_impl.hpp. We have typename MainModelType::SequenceIdx.
But now that the changes are done, I dont see it cluttering a lot.

nitbharambe · 2025-09-02T13:09:55Z

The build/tests seem to fail. I had cleaned up before pushing. Checking issues.

figueroa1395

I really like where this PR is going :). Very good ideas here.

figueroa1395 · 2025-09-03T08:07:58Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+    using TopologyTypesTuple =
+        std::tuple<std::type_identity<Node>, std::type_identity<Branch>, std::type_identity<Branch3>,
+                   std::type_identity<Source>, std::type_identity<Shunt>, std::type_identity<GenericLoadGen>,
+                   std::type_identity<GenericVoltageSensor>, std::type_identity<GenericPowerSensor>,
+                   std::type_identity<GenericCurrentSensor>, std::type_identity<Regulator>>;
+    // using TopologyConnectionTypes = std::tuple<std::type_identity<Branch>, std::type_identity<Branch3>,
+    // std::type_identity<Source>>;


Just a thought, but since this is very verbose/explicit, maybe it can be code generated so we don't forget to add stuff manually here. Or somehow link it to a list elsewhere, so we have only one explicit list, like in all_components.hpp. Just a thought for later cleanup.

figueroa1395 · 2025-09-03T08:08:41Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+template <typename Tuple> struct tuple_type_identities_to_tuple_types;
+
+template <typename... Ts> struct tuple_type_identities_to_tuple_types<std::tuple<std::type_identity<Ts>...>> {
+    using type = std::tuple<Ts...>;
+};


This is very cool!

figueroa1395 · 2025-09-03T08:09:39Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+template <class T, class U> struct MainModelType;
+
+template <class... ExtraRetrievableType, class... ComponentType>
+struct MainModelType<ExtraRetrievableTypes<ExtraRetrievableType...>, ComponentList<ComponentType...>> {


I don't mind this at all, I actually find it very neat.

figueroa1395 · 2025-09-03T08:11:42Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/topology.hpp

@@ -205,23 +205,24 @@ constexpr void register_connections_components(ComponentContainer components, Co
                                  [](Source const& source) { return source.status(); });
 }

+template <typename Tuple, std::size_t... Is>


Nitpick: Can you rename Is to something more expresive?

figueroa1395 · 2025-09-03T08:21:34Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+    using ComponentContainer = Container<ExtraRetrievableTypes<ExtraRetrievableType...>, ComponentType...>;
+    // using MainModelState = main_core::MainModelState<ComponentContainer>;


Maybe these two (included the commented out line) can potentially be used "more" in main_model_impl.hpp to clean it up a bit (there is some "duplication" in the other file now); just like it is done with MainModelType.

Signed-off-by: Nitish Bharambe <[email protected]>

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

figueroa1395 · 2025-09-08T07:17:34Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+    using ExtraRetrievableTypesTuple = std::tuple<std::type_identity<ExtraRetrievableType>...>;
+    using ComponentTypesTuple = std::tuple<ComponentType...>;


Should we make a distinction in the naming when something is just a tuple or a tuple of std::type_identitys? Or did you mean using ComponentTypesTuple = std::tuple<std::type_identity<ComponentType>...>;?

The alias here are the ones which shall be used outside. Ive changed the rest to *_ values signifying private members (and added private specifier too).

figueroa1395 · 2025-09-08T07:18:52Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+    // _AllTypesTupleUnique = typename tuple_type_identities_to_tuple_types<decltype(filter_tuple_types(
+    // _TopologyTypesTuple{}, _AllTypesTuple{}))>::type;
+
+    using _ComponentTypesTuple = std::tuple<std::type_identity<ComponentType>...>;


This and around, tied to f717e8c#r2329356329. Maybe _* is not descriptive enough when using std::type_identity in the tuples.

Marked them with private. They are named only to make the logic clear.

figueroa1395 · 2025-09-08T07:27:07Z

It looks like Sonar isn't identifying c++23 https://github.com/PowerGridModel/power-grid-model/actions/runs/17494449107/job/49691592601?pr=1109 (see complain about std::ranges::to)? Maybe it can be configured correctly? Perhaps #1116 will fix it.

c.c @mgovers

mgovers · 2025-09-08T07:41:41Z

It looks like Sonar isn't identifying c++23 https://github.com/PowerGridModel/power-grid-model/actions/runs/17494449107/job/49691592601?pr=1109 (see complain about std::ranges::to)? Maybe it can be configured correctly? Perhaps #1116 will fix it.

c.c @mgovers

that's exactly the intention of #1116 indeed 😬

figueroa1395 · 2025-09-08T07:50:00Z

tests/cpp_unit_tests/test_core_utils.cpp

+        static_assert(std::is_same_v<typename ModelType::TopologyConnectionTypesTuple, std::tuple<Branch, Source>>);
+        static_assert(ModelType::n_component_types == 3);
+    }
+    SUBCASE("Different component order: Line Source Node") {


So this one proves with the current implementation, the order doesn't matter, right? awesome!

static_asserts don't need to be in a subcase/test case (because they are static)

only that the main model template itself can be instantiated with arbitrary order, not that the model itself can be correctly created

static_asserts don't need to be in a subcase/test case (because they are static)

True. I understand that the subcase/test case is redundant here. Intention was to classify them instead of plain statements because I thought there would be way too many. What would be the way to go about classification?

figueroa1395 · 2025-09-08T07:51:26Z

tests/cpp_unit_tests/test_core_utils.cpp

+        static_assert(std::is_same_v<typename ModelType::TopologyConnectionTypesTuple, std::tuple<Branch, Source>>);
+        static_assert(ModelType::n_component_types == 3);
+    }
+    SUBCASE("Bad case: Line Source") {


Shouldn't this fail already as Node isn't present? shouldn't the model be not "constructible" according to our discussions?

I suppose the answer to this is related to #1109 (comment) item 2? It is still fine to instantiate this, but now we have to show that it can't be constructed? Would it be possible to make it fail at this stage, would that be desirable? I would guess so.

this is about whether the type is valid, not about whether the code actually works. if you want to make this static_asserts, you'll also have to make the constructor constexpr and actually instantiate

…experiment

Signed-off-by: Nitish Bharambe <[email protected]>

nitbharambe · 2025-09-09T14:48:22Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp

+    }
+};
+
+/////////////////// To remove ///////////////////


The usage of these utils with ComponentTypes... is for now (and mostly in future) limited only to ComponentTypes. So nice to just move them inside.

Signed-off-by: Nitish Bharambe <[email protected]>

…experiment Signed-off-by: Nitish Bharambe <[email protected]>

Signed-off-by: Nitish Bharambe <[email protected]>

nitbharambe added 2 commits September 2, 2025 14:36

experiment with topology

5a1fba0

Signed-off-by: Nitish Bharambe <[email protected]>

uncomment

dc02963

Signed-off-by: Nitish Bharambe <[email protected]>

nitbharambe self-assigned this Sep 2, 2025

nitbharambe added the improvement Improvement on internal implementation label Sep 2, 2025

nitbharambe added this to Power Grid Model Sep 2, 2025

nitbharambe removed this from Power Grid Model Sep 2, 2025

mgovers reviewed Sep 2, 2025

View reviewed changes

nitbharambe commented Sep 2, 2025

View reviewed changes

figueroa1395 reviewed Sep 3, 2025

View reviewed changes

mgovers mentioned this pull request Sep 3, 2025

Clean-up main model: fix windows clang-tidy #1111

Merged

2 tasks

nitbharambe added 2 commits September 5, 2025 15:16

add more questions

f717e8c

Signed-off-by: Nitish Bharambe <[email protected]>

add tests

8d00207

Signed-off-by: Nitish Bharambe <[email protected]>

figueroa1395 reviewed Sep 8, 2025

View reviewed changes

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/core_utils.hpp Outdated Show resolved Hide resolved

figueroa1395 reviewed Sep 8, 2025

View reviewed changes

mgovers and others added 5 commits September 8, 2025 11:16

Merge branch 'main' into feature/main-model-refactor-clean-templates-…

4840826

…experiment

Merge branch 'main' into feature/main-model-refactor-clean-templates-…

a01f8f3

…experiment

add all MainModelType features

c8d8512

Signed-off-by: Nitish Bharambe <[email protected]>

change to the generic tuple function

df398f1

Signed-off-by: Nitish Bharambe <[email protected]>

remove most variadics

9f196f5

Signed-off-by: Nitish Bharambe <[email protected]>

nitbharambe commented Sep 9, 2025

View reviewed changes

clean up core_utils

3495fe3

Signed-off-by: Nitish Bharambe <[email protected]>

This was referenced Sep 9, 2025

Cleanup main model: Clean up templates and variadics from y bus #1119

Draft

Type dependent checks: Checks at all components #1120

Draft

nitbharambe and others added 7 commits September 9, 2025 17:52

Merge branch 'main' into feature/main-model-refactor-clean-templates-…

9e54be9

…experiment Signed-off-by: Nitish Bharambe <[email protected]>

add tests

c997c1a

Signed-off-by: Nitish Bharambe <[email protected]>

change to sv

024be07

Signed-off-by: Nitish Bharambe <[email protected]>

rename

746264f

Signed-off-by: Nitish Bharambe <[email protected]>

move to new file

adfd800

Signed-off-by: Nitish Bharambe <[email protected]>

rename test file

63cdfcc

Signed-off-by: Nitish Bharambe <[email protected]>

address clang tidy

3b1358c

Signed-off-by: Nitish Bharambe <[email protected]>

		using ComponentContainer = Container<ExtraRetrievableTypes<ExtraRetrievableType...>, ComponentType...>;
		// using MainModelState = main_core::MainModelState<ComponentContainer>;

		using ExtraRetrievableTypesTuple = std::tuple<std::type_identity<ExtraRetrievableType>...>;
		using ComponentTypesTuple = std::tuple<ComponentType...>;

Cleanup main model: Clean up templates and variadics #1109

Are you sure you want to change the base?

Cleanup main model: Clean up templates and variadics #1109

Uh oh!

Conversation

nitbharambe commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitbharambe commented Sep 2, 2025

Uh oh!

figueroa1395 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

figueroa1395 commented Sep 8, 2025

Uh oh!

mgovers commented Sep 8, 2025

Uh oh!

figueroa1395 Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitbharambe Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nitbharambe commented Sep 2, 2025 •

edited

Loading

figueroa1395 Sep 8, 2025 •

edited

Loading

nitbharambe Sep 9, 2025 •

edited

Loading