Eigenvalues

[georgii-tishenin]

This is a pull request for the first prototype of eigenvalue extraction. The prototype supports eigenvalue extraction at every time step in EMT and DP domains for topologies containing single-phase R, L, C elements, switches and voltage sources.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

0.0% Coverage
2.1% Duplication

The version of Java (11.0.14.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[sonarcloud]

Quality Gate passed

Issues
24 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.5% Duplication on New Code

See analysis details on SonarCloud

[martinmoraga]

@georgii-tishenin Why do you have the two different classes EigenvalueCompInterface and EigenvalueDynamicCompInterface. Could these classes be merged?

[martinmoraga]

@georgii-tishenin I think the parameter mBranchIdx could be part of the class EigenvalueCompInterface or EigenvalueDynamicCompInterface because this parameter is a specific parameter of the eigenvalue extraction interface

[georgii-tishenin]

Hey @martinmoraga,
thank you for looking into the code and your comments! Here are my replies:

1. @georgii-tishenin Why do you have the two different classes EigenvalueCompInterface and EigenvalueDynamicCompInterface. Could these classes be merged?

There are dynamic components, like inductor or capacitor, and non-dynamic components, like resistor, switch or voltage source.
To build the matrices for eigenvalue extraction all components need to stamp a value into the branchNodeIncidenceMatrix. For that reason I have introduced EigenvalueCompInterface, that needs to be implemented by all components (supported by eigenvalue extraction).
Dynamic components need to stamp a value into signMatrix and discretizationMatrix additionally. For that reason, I have introduced EigenvalueDynamicCompInterface, that inherits from EigenvalueCompInterface . This way dynamic components need to implement only EigenvalueDynamicCompInterface, and due to inheritance they are forced to implement the functions of EigenvalueCompInterface as well. Hope I could explain the idea alright.

2. @georgii-tishenin I think the parameter mBranchIdx could be part of the class EigenvalueCompInterface or EigenvalueDynamicCompInterface because this parameter is a specific parameter of the eigenvalue extraction interface

Yes, @dinkelbachjan actually had a similar comment. This is what I replied:

Thank you for the idea! ChatGPT and me would rather not add private data members to an interface 😃, although C++ does not restrict it.

For example, see Learn C++ - Pure virtual functions, abstract base classes, and interface classes :

An interface class is a class that has no member variables, and where all of the functions are pure virtual! Interfaces are useful when you want to define the functionality that derived classes must implement, but leave the details of how the derived class implements that functionality entirely up to the derived class.

May be we can find a different solution, e.g., composition over inheritance (as you proposed), or moving mBranchIdx to a base class. But for now I would leave it as it is, if you agree.

So, if you guys insist, I can do it. But personally I would prefer keeping the interface without data members.

[martinmoraga]

inheritance they are forced to implement the functions of EigenvalueCompInterface as well. Hope I could explain the idea alright.

@georgii-tishenin

yes, I know what you mean but C++ doesn't have interfaces "enforced by the language", so I my opinion you can define these classes like normal class definition without any special rules.

Another option is to move this variable (and his setter) to dpsim/dpsim-models/include/dpsim-models/SimPowerComp.h

[martinmoraga]

@georgii-tishenin

Regarding the classes EigenvalueCompInterface and EigenvalueDynamicCompInterface. You could declare the functions declared in
EigenvalueDynamicCompInterface in the class EigenvalueCompInterface as empty virtual functions and not as pure virtual functions. Then you can overwrite these functions in the dynamic components, but the "static components" will call the empty version of these function. E.g.:

  virtual void stampSignMatrix(MatrixVar<VarType> &signMatrix,
                               Complex coeffDP) { };

It is only an idea to avoid having different interfaces for one "solver"... what do you think about that?

[martinmoraga]

@georgii-tishenin

we have a macro defining the "tolerance" to compare doubles.

dpsim/dpsim/include/dpsim/Definitions.h

Line 14 in 0f4d17c

#define DOUBLE_EPSILON 1E-12

[georgii-tishenin]

nice to know, will use it👍

[martinmoraga]

@georgii-tishenin

you could add to the docu which components are supported by the eigenvalue extraction "solver"

[georgii-tishenin]

@georgii-tishenin

you could add to the docu which components are supported by the eigenvalue extraction "solver"

yes, that makes sense👍. Will add that.

[georgii-tishenin]

inheritance they are forced to implement the functions of EigenvalueCompInterface as well. Hope I could explain the idea alright.

@georgii-tishenin

yes, I know what you mean but C++ doesn't have interfaces "enforced by the language", so I my opinion you can define these classes like normal class definition without any special rules.

Another option is to move this variable (and his setter) to dpsim/dpsim-models/include/dpsim-models/SimPowerComp.h

I personally like the 2nd option you propose better: to move the variable mBranchIdx to the SimPowerComp base class. @martinmoraga, shall I do it?
The only downside I see is that mBranchIdx is not used by all of the classes derived from SimPowerComp.

[georgii-tishenin]

@georgii-tishenin

Regarding the classes EigenvalueCompInterface and EigenvalueDynamicCompInterface. You could declare the functions declared in EigenvalueDynamicCompInterface in the class EigenvalueCompInterface as empty virtual functions and not as pure virtual functions. Then you can overwrite these functions in the dynamic components, but the "static components" will call the empty version of these function. E.g.:

  virtual void stampSignMatrix(MatrixVar<VarType> &signMatrix,
                               Complex coeffDP) { };

It is only an idea to avoid having different interfaces for one "solver"... what do you think about that?

Thanks for the idea! I think these two options are equivalent, I sort of like the option with EigenvalueCompInterface and EigenvalueDynamicCompInterface better, because it also clearly groups the components, implementing the interfaces. You do not get the grouping with just one interface. But is there any special reason to avoid having two different interfaces?

[sonarcloud]

Quality Gate passed

Issues
25 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.7% Duplication on New Code

See analysis details on SonarCloud

[MarvinTollnitschRWTH]

This is done in the other component interfaces as well, but generally I think it is better to be explicit about data types. It might not be clear that EigenvalueCompInterface::List is a vector of shared pointers of exactly the class the type was defined in.

It might be hard to understand since EigenvalueCompInterface::List could be any "list"-type containing anything within the interface class. However, once you do know what it means and use it often it may improve readability. I still vote against it though personally.

[georgii-tishenin]

thanks for your feedback! I agree with your arguments. In this particular case even the actual type would not read too bad std::vector<std::shared_ptr<EigenvalueCompInterface>>. However, if there were no aliases at all, codebase in general would become less readable. So, for now let's just keep consistent with the other component interfaces.

[sonarcloud]

Quality Gate passed

Issues
26 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.4% Duplication on New Code

See analysis details on SonarCloud

[m-mirz]

@georgii-tishenin I also prefer interface classes without member variables. One issue that we have in dpsim is a deep inheritance hierarchy, each hierarchy level adding new member variables.
Due to this we often had discussions on how to avoid yet another diamond shape inheritance which causes confusing members.
We should favor interfaces and composition over inheritance.

[georgii-tishenin]

@m-mirz, thanks for your feedback! I will keep interfaces without member variables.
I am now refactoring after comments from @martinmoraga and @dinkelbachjan. And will soon convert this draft PR back to standard PR.

[sonarcloud]

Quality Gate passed

Issues
25 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.3% Duplication on New Code

See analysis details on SonarCloud