feat: harmonics-domain support for CompositePowerComp - #557
feat: harmonics-domain support for CompositePowerComp#557leonardocarreras wants to merge 11 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #557 +/- ##
==========================================
+ Coverage 72.14% 73.07% +0.92%
==========================================
Files 491 491
Lines 31623 31748 +125
Branches 16950 17007 +57
==========================================
+ Hits 22815 23199 +384
+ Misses 8722 8485 -237
+ Partials 86 64 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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: Leonardo Carreras <leonardo.carreras@eonerc.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: Leonardo Carreras <leonardo.carreras@eonerc.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: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
9699e31 to
a26b60b
Compare
DPsim LLM reviewClaim vs. code: matches the description. Found 1 critical, 3 medium (2 anchored to lines below). 🔴 Critical & high
🟡 Suggestions
🔵 Optional / low-confidence (2)
Claim vs. implementation
How this review was produced13 specialized finder passes raised 47 findings over the diff and the full changed sources. After de-duplication, 47 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 43 refuted as unsupported, 4 kept (2 tentative). Refuted by verification:
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.
TL;DR: One real numerical safety issue remains in the new harmonics path: the DP resistor current update can divide by zero, while the other reported items are low-confidence or duplicate documentation nits and the solver-path concerns are already addressed by the PR context. Overall risk is moderate and concentrated in one component-level guard, with no additional confirmed correctness issues surfaced in the merged findings.
Found 1 high, 1 medium (2 anchored to lines below).
🔴 Critical & high
- Missing null/zero check for resistance before division in mnaCompUpdateCurrentHarm
[high · 92% confidence]indpsim-models/src/DP/DP_Ph1_Resistor.cpp:165(details inline)
🟡 Suggestions
- Potential out-of-bounds access in mnaCompInitializeHarm if leftVectors is empty
[medium · 80% confidence]indpsim-models/src/CompositePowerComp.cpp:158(details inline)
Claim vs. implementation
- Claimed: Add harmonics-domain support for CompositePowerComp, including DP transformer support, per-frequency R/L/C current updates, solver-path fixes, and a transformer harmonics example/notebook.
- Done: Implements harmonics-domain hooks for CompositePowerComp and DP transformer, adds per-frequency current/voltage update hooks for DP passive parts, fixes frequency-parallel solver matrix handling and left-vector initialization, and adds a new example plus notebook.
- Difference: none
How this review was produced
13 specialized finder passes raised 83 findings over the diff and the full changed sources. After de-duplication, 83 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 77 refuted as unsupported, 6 kept (4 tentative).
Refuted by verification:
- Incorrect voltage update formula in Transformer::mnaCompUpdateVoltageHarm (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): mnaCompUpdateVoltageHarm computes v1 then subtracts mVirtualNodes[0] at lines 338-343, so it does not overwrite the difference with zero
- Redundant zero assignment before voltage difference computation (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): The zero assignment at line 337 is immediately overwritten by the node-voltage read and subtraction, matching the same pattern used in mnaCompUpdateVoltage
- Incorrect voltage update in mnaCompUpdateVoltageHarm (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): The harmonic voltage update ends with (**mIntfVoltage)(0, freqIdx) = v1 - v0, not v1 alone
- Redundant zero assignment in mnaCompUpdateVoltageHarm (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): The initial zeroing is redundant but intentional and already present in the non-harmonic voltage update pattern
- Missing null/zero check for capacitance in mnaCompUpdateCurrentHarm (dpsim-models/src/DP/DP_Ph1_Capacitor.cpp): The code already computes zero-current safely via mEquivCond and the initialization comment explicitly notes zero capacitance gives 0 current, not NaN.
- Missing null/zero check for inductance in mnaCompUpdateCurrentHarm (dpsim-models/src/DP/DP_Ph1_Inductor.cpp): The new harmonic update just multiplies by mEquivCond and mEquivCurrent; no zero/negative-inductance check is introduced here, and the same inductance-dependent math already exists elsewhere in initVars/tear code.
- Missing null check for mSubInductor before dereference in mnaCompUpdateCurrentHarm (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): mnaCompUpdateCurrentHarm is only called after createSubComponents/addMNASubComponent has created mSubInductor, and the file shows no alternate path that leaves it null
- Missing null check for mVirtualNodes[0] before dereference in mnaCompUpdateVoltageHarm (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): mnaCompUpdateVoltageHarm uses mVirtualNodes[0] after createSubComponents sets up the virtual nodes, so the file does not show a missing-null-check defect
- Use of ad-hoc literal 1.0 instead of DOUBLE_EPSILON for frequency-independent transformer stamp (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): Complex(-1.0, 0) and Complex(1.0, 0) are the correct ideal-transformer stamp constants here, not tolerance comparisons
- Ideal transformer harmonic stamp uses frequency-independent equations without documenting assumption (dpsim-models/src/DP/DP_Ph1_Transformer.cpp): The code explicitly documents the assumption with the comment 'Ideal transformer equations, frequency-independent'
- Add missing dependency declarations for MnaPreStepHarm and MnaPostStepHarm tasks (dpsim-models/include/dpsim-models/CompositePowerComp.h): Task already exposes getAttributeDependencies()/getPrevStepDependencies() in Task.h, and these constructors populate mPrevStepDependencies/mAttributeDependencies/mModifiedAttributes.
- Missing mnaCompAddPreStepDependencies override in CompositePowerComp (dpsim-models/src/CompositePowerComp.cpp): CompositePowerComp.h already declares mnaCompAddPreStepDependencies and this .cpp defines it at lines 120-130.
- Missing mnaCompAddPostStepDependencies override in CompositePowerComp (dpsim-models/src/CompositePowerComp.cpp): CompositePowerComp.h already declares mnaCompAddPostStepDependencies and this .cpp defines it at lines 133-145.
- Capacitor::MnaPreStepHarm::execute reads mRightVector without declared dependency (dpsim-models/src/DP/DP_Ph1_Capacitor.cpp): MnaPreStepHarm::execute only uses mRightVector, and the constructor in the header already declares it as a modified attribute.
- Capacitor::MnaPostStepHarm::execute modifies mIntfVoltage and mIntfCurrent without declared writes (dpsim-models/src/DP/DP_Ph1_Capacitor.cpp): MnaPostStepHarm::execute updates interface state, and the constructor already declares v_intf and i_intf as modified attributes.
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.
| } | ||
|
|
||
| void DP::Ph1::Resistor::mnaCompUpdateCurrentHarm(Int freqIdx) { | ||
| (**mIntfCurrent)(0, freqIdx) = (**mIntfVoltage)(0, freqIdx) / **mResistance; |
There was a problem hiding this comment.
Missing null/zero check for resistance before division in mnaCompUpdateCurrentHarm
severity: high · confidence: 92%
The method mnaCompUpdateCurrentHarm(Int freqIdx) divides the interface voltage by **mResistance without validating that **mResistance is not zero or near-zero. In the DP domain, a zero-valued resistance would lead to division by zero and NaN/Inf propagation, corrupting the admittance matrix and subsequent solves.
Suggested fix: Add a guard using Math::isFinite and a tolerance check against DOUBLE_EPSILON before the division. If the resistance is non-positive or too small, throw std::invalid_argument with a clear message so the model fails fast rather than running silently wrong.
Checked against the source: mnaCompUpdateCurrentHarm divides by **mResistance with no zero or near-zero guard
stage: numerics
| } | ||
|
|
||
| **this->mRightVector = | ||
| Matrix::Zero(leftVectors[0]->get().rows(), this->mNumFreqs); |
There was a problem hiding this comment.
Potential out-of-bounds access in mnaCompInitializeHarm if leftVectors is empty
severity: medium · confidence: 80%
The method mnaCompInitializeHarm accesses leftVectors[0] without checking whether leftVectors is empty. If the caller passes an empty vector, this results in undefined behavior and likely a crash.
Suggested fix: Add an assertion or guard to ensure leftVectors.size() > 0 before accessing leftVectors[0].
| Matrix::Zero(leftVectors[0]->get().rows(), this->mNumFreqs); | |
| if (leftVectors.empty()) { | |
| throw std::invalid_argument("leftVectors must not be empty in mnaCompInitializeHarm"); | |
| } | |
| **this->mRightVector = Matrix::Zero(leftVectors[0]->get().rows(), this->mNumFreqs); |
Checked against the source: mnaCompInitializeHarm indexes leftVectors[0] without any empty check in this function
stage: numerics
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Summary
CompositePowerComphad no harmonics-domain (parallel-frequency) hooks, so a composite component like the DP transformer could not be used withdoFrequencyParallelization(true). This adds that support and wires upDP::Ph1::Transformeras the first consumer, plus the per-frequency current update needed on its DPResistor/Inductor/Capacitorsubcomponents.Scoping and testing this surfaced several independent, pre-existing bugs in the harmonics-parallel solver path that had never been exercised end-to-end before (no committed example combines frequency-parallel mode with a real per-step logger). All are fixed here:
mLeftSideVectorHarmwas populated twice during init, leaving it at double the real frequency count.makeCompressed(), so KLU read a malformed matrix and silently no-op'd instead of solving.SolveTaskHarmincorrectly declared that it modifies node voltage (copied from the non-HarmSolveTask, where that's true but here it isn't), confusing the scheduler's dependency graph.