Fix component initialization in pi lines, RX loads - #697
Conversation
052099d to
e73ad45
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: One high-confidence bug remains in EMT Ph1 PiLine where initialization can divide by zero, while the transformer and RXLoad changes are mostly covered by lower-confidence test/documentation gaps and one unconfirmed DP RXLoad equation concern; the scheduler and powerflow guard changes did not surface additional confirmed correctness issues.
Found 1 high, 1 medium (2 anchored to lines below).
🔴 Critical & high
- Division by possibly zero impedance
[high · 92% confidence]indpsim-models/src/EMT/EMT_Ph1_PiLine.cpp:105(details inline)
🟡 Suggestions
- Missing snubber capacitance initialization guard in EMT Ph3 Transformer
[medium · 80% confidence]indpsim-models/src/EMT/EMT_Ph3_Transformer.cpp:175(details inline)
Not shown: 15 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Fix initialization issues in EMT pi lines, EMT/DP RX loads, transformers, and powerflow generator lookup.
- Done: Adds guards and corrected seeding for EMT Ph1 pi line and EMT Ph3 RX load initialization, fixes DP Ph1 RX load clone/current handling, skips transformer snubbers when rated power is non-positive, and null-checks generator lookup in initWithPowerflow.
- Difference: none
How this review was produced
13 specialized finder passes raised 91 findings over the diff and the full changed sources. After de-duplication, 91 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 74 refuted as unsupported, 17 kept (15 tentative).
Refuted by verification:
- Incorrect virtual node initialization in EMT Ph1 PiLine (dpsim-models/src/EMT/EMT_Ph1_PiLine.cpp): The file already seeds the virtual node with
initialSingleVoltage(0) + PEAK1PH_TO_RMS3PH * current * **mSeriesRes, andcurrentis derived from the peak line-to-line voltage at line 105. - Incorrect impedance matrix inversion in EMT Ph3 RXLoad initialization (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The code explicitly builds a complex 3x3 impedance from R and X and inverts it as
impedance.inverse() * vInitABC. - Incorrect virtual node initialization in EMT Ph3 RXLoad series mode (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The file uses
PEAK1PH_TO_RMS3PH * (vInitTerm0 - mResistance * iInit)exactly as described, and the finding itself says this sign is correct. - Incorrect conjugate power flow sign in DP Ph1 RXLoad initialization (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The code uses std::conj(Complex(**mActivePower, **mReactivePower) / (**mIntfVoltage)(0, 0)), which is the passive-sign-convention form for load current initialization.
- Incorrect unit conversion for EMT Ph1 PiLine virtual node seed (dpsim-models/src/EMT/EMT_Ph1_PiLine.cpp): The claimed mixed-unit seed is exactly the changed line, and the code intentionally converts the peak voltage drop back to RMS3PH before adding it to
initialSingleVoltage(0). - ... and 10 more, in the workflow log.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
Found 1 critical, 2 high (3 anchored to lines below).
🔴 Critical & high
- Division by possibly zero impedance
[critical · 90% confidence]indpsim-models/src/EMT/EMT_Ph1_PiLine.cpp:105(details inline) - Use DOUBLE_EPSILON for near-zero voltage guard in DP_Ph1_RXLoad
[high · 90% confidence]indpsim-models/src/DP/DP_Ph1_RXLoad.cpp:90(details inline) - Use tolerance instead of zero check for voltage division
[high · 88% confidence]indpsim-models/src/DP/DP_Ph1_RXLoad.cpp:90(details inline)
Not shown: 40 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Fix initialization bugs in pi lines, RX loads, transformers, and powerflow generator lookup.
- Done: Adds guards and corrected seeding/initialization for EMT Ph1 pi lines, EMT Ph3 and DP Ph1 RX loads, DP/EMT transformers, and null-safe generator lookup in powerflow init.
- Difference: none
How this review was produced
13 specialized finder passes raised 117 findings over the diff and the full changed sources. After de-duplication, 117 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 74 refuted as unsupported, 43 kept (40 tentative).
Refuted by verification:
- Incorrect per-phase impedance scaling in EMT Ph3 RXLoad (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The code intentionally uses per-phase values from the diagonal matrices and the diff only changed the zero-case handling, not the impedance formula.
- Incorrect virtual-node voltage seed in EMT Ph3 RXLoad series mode (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The series-mode seed line is present as the intended implementation, and the surrounding code already converts terminal voltage to peak units before applying the virtual-node seed.
- Incorrect matrix dimension for zero reactance in EMT Ph3 RXLoad (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): mReactance is initialized to Matrix::Zero(3, 3), not a 1x1 matrix, so the claimed dimension mismatch is absent.
- Incorrect sign in virtual node voltage seed for EMT Ph3 RXLoad series mode (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The minus sign is the actual code and matches the PR change from the previous plus-sign formulation.
- Missing snubber capacitance initialization guard for mSubSnubCapacitor1 (dpsim-models/src/EMT/EMT_Ph3_Transformer.cpp): The code already guards the capacitor setup with
if (mSubSnubCapacitor2)before callingsetParameters. - ... and 10 more, in the workflow log.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #697 +/- ##
==========================================
- Coverage 64.94% 64.75% -0.20%
==========================================
Files 533 533
Lines 37480 37485 +5
Branches 19933 19935 +2
==========================================
- Hits 24342 24274 -68
- Misses 13090 13125 +35
- Partials 48 86 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e73ad45 to
49d5683
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: The diff also removes a duplicated EMT Ph3 RX-load impedance computation and changes its series-mode virtual-node seed formula/sign, which are not separately mentioned in the title and are only partially covered by the description.
TL;DR: Four real correctness issues remain: singular-matrix/zero-impedance initialization paths in EMT Ph3 RXLoad and EMT Ph1 PiLine, plus the DP Ph1 RXLoad divide-by-zero/clone behavior; the rest are lower-value documentation/test coverage gaps or duplicate requests, and the topology null-lookup fix appears unflagged here.
Found 2 high (2 anchored to lines below).
🔴 Critical & high
- Missing guard for singular impedance matrix inversion
[high · 94% confidence]indpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp:199(details inline) - Guard matrix inversion for resistance calculation
[high · 90% confidence]indpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp:199(details inline)
Not shown: 9 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Fix initialization bugs in EMT pi lines and RX loads, plus DP RX load and powerflow generator lookup guards.
- Done: Updates EMT Ph1 pi-line and EMT Ph3 RX-load virtual-node/current initialization, honors RX-load log level, guards DP Ph1 RX-load clone/current initialization, and skips missing generator-backed components in powerflow init.
- Difference: The diff also removes a duplicated EMT Ph3 RX-load impedance computation and changes its series-mode virtual-node seed formula/sign, which are not separately mentioned in the title and are only partially covered by the description.
How this review was produced
13 specialized finder passes raised 75 findings over the diff and the full changed sources. After de-duplication, 75 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 61 refuted as unsupported, 14 kept (11 tentative).
Refuted by verification:
- Incorrect complex power sign convention in DP Ph1 RXLoad initialization (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The current formula is exactly the existing initialization code, and the file already guards zero voltage before the division.
- Incorrect virtual-node voltage seed in EMT Ph1 PiLine (dpsim-models/src/EMT/EMT_Ph1_PiLine.cpp): The seed uses
PEAK1PH_TO_RMS3PH * current * **mSeriesRes, and the file’s owninitialSingleVoltage()/voltagesetup shows the initialization is intentionally converted before assignment. - Fix clone to respect parameterization state (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The clone method now explicitly guards setParameters with
if (mParametersSet)on the changed line. - Correct virtual node initialization units in EMT Ph1 PiLine (dpsim-models/src/EMT/EMT_Ph1_PiLine.cpp): The code does not convert the current term back to RMS3PH twice; it computes
current = voltage / impedanceonce and applies a singlePEAK1PH_TO_RMS3PHfactor at line 111. - Honour logLevel argument in EMT Ph3 RXLoad constructor (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp): The constructor now forwards the provided logLevel to CompositePowerComp(..., logLevel).
- ... and 10 more, in the workflow log.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: matthiasmees <matthias.mees@rwth-aachen.de> Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: matthiasmees <matthias.mees@rwth-aachen.de> Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: matthiasmees <matthias.mees@rwth-aachen.de> Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
49d5683 to
b14aaa8
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: Two high-severity numerical robustness issues remain in EMT::Ph1::PiLine and EMT::Ph3::RXLoad, while the rest are lower-confidence documentation/test coverage gaps around the initialization fixes; the DP RXLoad and SystemTopology changes did not surface a confirmed correctness bug here.
Found 2 high, 1 medium (1 anchored to lines below).
🔴 Critical & high
- Potential division by zero when series impedance is zero
[high · 92% confidence]indpsim-models/src/EMT/EMT_Ph1_PiLine.cpp:105(details inline) - Potential singular matrix inversion for active power
[high · 90% confidence]indpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp:130
The code computesmResistance = pow(**mNomVoltage / sqrt(3), 2) * (**mActivePower).inverse();after only checking that (**mActivePower)(0,0) != 0. If any other diagonal element of the 3×3 active power matrix is zero (or the matrix is otherwise singular),Matrix::inverse()will throw or produce NaNs, leading to a crash or invalid impedance values during simulation.
Fix: Guard the inversion by checking that the entire active-power matrix is invertible (e.g. usingEigen::FullPivLUto test rank) or compute the impedance per-phase using only the diagonal entries. If the matrix is singular, set the corresponding resistance entries to zero or raise a clear error.
🟡 Suggestions
- Near-zero power matrix check uses single element comparison
[medium · 90% confidence]indpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp:130
Impedance and resistance calculations are guarded only by checking( **mActivePower )(0,0) != 0. If other diagonal entries are zero or the matrix is singular, the subsequentinverse()call can produce NaNs or throw, corrupting the admittance matrix.
Fix: Validate the entire active-power matrix (e.g., check its determinant or that all diagonal entries exceed a tolerance) before callinginverse(). If the matrix is singular, set resistance to a zero matrix and log a warning.
Not shown: 9 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Fix initialization bugs in EMT pi lines and RX loads, plus DP RX load and powerflow generator lookup guards.
- Done: Updates EMT Ph1 pi-line and EMT Ph3 RX-load initialization math, honors RX-load log level, fixes DP Ph1 RX-load clone/current initialization, and skips missing generator components in powerflow init.
- Difference: none
How this review was produced
13 specialized finder passes raised 102 findings over the diff and the full changed sources. After de-duplication, 102 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 89 refuted as unsupported, 13 kept (10 tentative).
Refuted by verification:
- Incorrect complex power sign convention in DP Ph1 RXLoad initialization (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The file intentionally initializes current as std::conj(Complex(P,Q)/V) on line 91, matching the existing load initialization pattern rather than a missing or absent implementation.
- Inconsistent zeroing of mReactance matrix in EMT Ph3 RXLoad (dpsim-models/src/EMT/EMT_Ph3_RXLoad.cpp):
mReactanceis zeroed in the else branch at line 138, so it is not left stale when reactive power is zero. - Incorrect scaling of initial current in DP Ph1 RXLoad (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The expression on lines 89-93 is a direct complex-power-to-current initialization; no extra RMS scaling factor is present or implied in the code.
- Missing null check before dereferencing generator component in initWithPowerflow (dpsim-models/src/SystemTopology.cpp): The added null check is on the looked-up component
compbeforecomp->terminals()[0]; the cast toSynchronGeneratoris already guarded byif (auto genPF = std::dynamic_pointer_cast<...>(compPF))on lines 104-105. - Replace ad-hoc zero check with DOUBLE_EPSILON guard (dpsim-models/src/DP/DP_Ph1_RXLoad.cpp): The changed guard explicitly uses std::abs((**mIntfVoltage)(0, 0)) > 0 on line 90, so this is not an ad-hoc literal zero check claim against the current file.
- ... and 10 more, in the workflow log.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
8aac114
into
sogno-platform:master
|



Unrelated in relation to components, but all related to initialization.
EMT::Ph1::PiLineseeded its virtual node with a peak single-phase term added onto an RMS line-line voltage, phase discarded.EMT::Ph3::RXLoadhardcodedLogger::Level::trace, so impossible to turn logging off.DP::Ph1::RXLoaddivided by an unchecked terminal voltage, andclone()marked unparameterized copies as parameterized.SystemTopology::initWithPowerflow()dereferenced a null generator lookup.For some fixes related to RX loads, thanks to Matthias.