Fix EMT 3Ph Switch behaviour for Matrix Recomputation

[FGH-wirtz]

• proposed fix for Issue Switch / Recompute not performed for Switch with sim.doSystemMatrixRecomputation(true); #415
• Added new Testcase for EMT_Ph3_Switch
• changed EMT/EMT_Ph3_Switch.h to MNAVariableCompInterface
• added Bool EMT::Ph3::Switch::hasParameterChanged() implementation
• new member variable for Switch: protected: Bool mPrevState = false;

[leonardocarreras]

Hi @cwirtz-fgh, thanks for your contribution

We can see that the results of the notebooks that use the results from the faults are having some errors in the value they use as comparison. Do you have an opinion or explanation for this? It looks like the value at the point where we assert is not near enough to zero, and before it was matching the tolerance.

[FGH-wirtz]

Seems to be an issue in the testcases with VBR which do a recomputation every timestep. With my proposed fix, the Switch is handled as an VariableComp as well as an SwitchComp and stamped twice in recomputeSystemMatrix - which seems to cause the issue. I will test this more thoroughly.

[FGH-wirtz]

Issue is confirmed in tests, calling mnaApplySystemMatrixStamp twice for the switch component changes the system matrix and causes this behaviour.
This leads to the following questions / options:

• Options to fix this:
• 1. Switches are NO variableComp --> implement an equivalent function to hasVariableComponentChanged() e.g. as hasSwitchComponentChanged(), so a recomputation can be triggered for switches
• 2. Switches ARE implemented as varComp --> in recomputeSystemMatrix, switches should not be handled seperately as they are already restamped as varComp

From my perspective option 2 is preferrable.

Even though there is no testcase implemented for this, the problem should also affect varResSwitch, as this component has a switchComp as well as an varComp interface.

[codecov]

Codecov Report

❌ Patch coverage is 23.80952% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.79%. Comparing base (bafb741) to head (3ff63fd).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
dpsim-models/src/DP/DP_Ph1_Switch.cpp	0.00%	5 Missing ⚠️
dpsim-models/src/DP/DP_Ph3_SeriesSwitch.cpp	28.57%	5 Missing ⚠️
dpsim-models/src/EMT/EMT_Ph1_Switch.cpp	0.00%	5 Missing ⚠️
dpsim-models/src/EMT/EMT_Ph3_SeriesSwitch.cpp	0.00%	5 Missing ⚠️
dpsim-models/src/EMT/EMT_Ph3_Switch.cpp	0.00%	5 Missing ⚠️
dpsim-models/src/SP/SP_Ph1_Switch.cpp	0.00%	5 Missing ⚠️
dpsim-models/src/DP/DP_Ph1_RXLoadSwitch.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   65.71%   65.79%   +0.07%     
==========================================
  Files         381      381              
  Lines       23418    23445      +27     
  Branches    11696    11698       +2     
==========================================
+ Hits        15390    15425      +35     
- Misses       7953     7976      +23     
+ Partials       75       44      -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leonardocarreras]

Hi @cwirtz-fgh!
Thanks again for the PR. I also prefer the option 2, it looks better to me to unify the way components work. Thanks for doing it for the 1ph and 3ph switches already.
I will try to have a look in the next days, hopefully also some others can do do the same. Maybe there is some use case that needs the previous behavior.

[leonardocarreras]

Thanks for the PR!
In general, the idea and implementation looks good to me.

What we could improve:

1. Move the common code to Base class for single phase and three phase
2. When moving, particularly the ones to move are the protected attribute holding the previous closed state and the new overriding function to determine change in parameters to the base class (as all switches are similar).
3. Might be a nice extra to put a more meaningful name than Bool mPrevState to hold the closed status, for example Bool mIsClosedPrev (this can also be something to refactor together at a later stage with all the other switches in the codebase that are currently using the same too)

[leonardocarreras]

Just as extra (but a bit old) context, we may also want to have a look at #59 to understand more what else could be impacted here and if this should be labeled as breaking, or if we have some transition with a "deprecated version".

[leonardocarreras]

Looks good to me, might need rebasing