Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Address the component initialization paradox #574

Open
Jerry-Jinfeng-Guo opened this issue Apr 20, 2024 · 2 comments
Open

[FEATURE] Address the component initialization paradox #574

Jerry-Jinfeng-Guo opened this issue Apr 20, 2024 · 2 comments
Labels
feature New feature or request

Comments

@Jerry-Jinfeng-Guo
Copy link
Contributor

Jerry-Jinfeng-Guo commented Apr 20, 2024

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 CompInputs).

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.

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the feature New feature or request label Apr 20, 2024
@mgovers
Copy link
Member

mgovers commented Apr 22, 2024

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 #562 (comment) .

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
    }
};

@Jerry-Jinfeng-Guo
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Status: No status
Development

No branches or pull requests

2 participants