power-grid-model
power-grid-model copied to clipboard
[FEATURE] Address the component initialization paradox
Status-quo
For almost all components, we have certain Comp
, with default constructor taking in a CompInput
as input, next to a variety of other parameters, e.g., double u_rated
. In this case, my interpretation is that the args after CompInput
(e.g., rated voltage) is not part of the intrinsic parameterization of the component but requirement on another fold.
For instance, a Transformer
component is constructed as follows:
Transformer(TransformerInput const& transformer_input, double u1_rated, double u2_rated)
Why am I complaining?
In almost all test cases, we never have to construct a multitude of certain component, i.e., std::vector<Comp>
. However, the container support adding multiple components (of same type) in one go by calling main_core::add_component<Comp>(*, std::vector<CompInput>::begin(), std::vector<CompInput>::end(), *)
. Since there were no such thing as a default value across all components (constructors), said add_component
function would fail.
My argument 1
Member variables living inside Comp
are intrinsic by nature, and when instantiated all parameters contributing to them should live inside CompInput
.
Proposal 1
In short, we ditch the logic of distinguishing intrinsic and extrinsic parameters (in all constructors and CompInput
s).
My argument 2
Assumption that all data would be passed from Python
side with all the good properties should be questioned.
Proposal 2
In short, add default values to (at least part of) constructor parameters.
I do agree that the current state (and mainly the syntax) makes it less readable and testable. I propose going with Proposal 2 with a slight modification:
Argument against Proposal 1
My argument against Proposal 1: we should not expect our C API users to be able to do the translation from p.u. to unit-full, especially because the units may differ on a node-by-node basis, which would be hard in the case of Proposal 1.
Argument against Proposal 2 with default values
The major downside with using default values is the fact that it's quite error-prone during future refactorings (for instance, we will not be notified at all usages when u1_rated <-> u2_rated
are swapped).
Proposal 2b: using overloads
Instead of adding default values, I would make an explicit constructor overload that has unit 1
. That constructor would then use the other constructor (or vice versa). See also https://github.com/PowerGridModel/power-grid-model/pull/562#discussion_r1574196049 .
The main advantage of that is that it is forbidden to e.g. only provide 1 of the arguments, making it more future-proof. In addition, any changes to the current constructor remain localized and do not affect tests for random other parts of the code because they keep using the same constructor.
Proposal 3: explicit dependent parameters
We could also make the distinction between intrinsic and extrinsic parameters even more explicit by creating a separate struct:
struct CompInput {
// intrinsic
};
struct CompDependentInput {
// extrinsic (with good default values)
};
class Comp {
public:
explicit Comp(CompInput const& intrinsics): Comp{intrinsics, CompDependentInput{}} {} // calls the one below
explicit Comp(CompInput const& intrinsics, CompDependentInput const& extrinsics) {
// body
}
};
Argument against Proposal 1
My argument against Proposal 1: we should not expect our C API users to be able to do the translation from p.u. to unit-full, especially because the units may differ on a node-by-node basis, which would be hard in the case of Proposal 1.
I could definitely use a crush course on the exchanges over proposal 1. Let's sort it out during one of the knowledge sharing or refinement.