feat: Variable time step for the MNA solver (DP domain) - #701
Conversation
setTimeStep() is init-only: components derive their companion models from dt in mnaCompInitialize() and keep the result, so writing dt later leaves every discretisation on the old step. A new step now travels from the caller down to the components. Simulation::updateTimeStep() writes it and calls every solver, MnaSolver asks each of its components through the new MNAInterface::mnaUpdateTimeStep(), and Solver::updateTimeStep() is the base for solvers whose components hold no discretisation and only records it. A component returns false when it cannot follow; the count goes back to the caller and the declining types are named in the log, because a stale discretisation leaves the run plausible. No component implements it for this first commit, so a call declines everything and changes nothing. MnaSolverDirect also re-stamps the base matrix, which carries the static components with the old dt and is refreshed nowhere else, and factorises fully rather than through partialRefactorize(). With precomputed switch matrices the change cannot be followed and is refused with a warning. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
leonardocarreras
added
enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: Three real issues stand out: the new DP Ph3 passive-element time-step hooks are missing state needed to reinitialize correctly, while the rest of the report is mostly low-confidence or contradicted speculation about docs, overrides, and scheduler behavior. The main risk is incorrect variable-step behavior in Ph3 inductors/capacitors if the stored frequency/time-step state is incomplete; the remaining findings are either unconfirmed or not supported by the shown code.
Found 2 medium (2 anchored to lines below).
🟡 Suggestions
- Add mTimeStep tracking to DP Ph3 Inductor
[medium · 68% confidence]indpsim-models/include/dpsim-models/DP/DP_Ph3_Inductor.h:39(details inline) - Add mTimeStep tracking to DP Ph3 Capacitor
[medium · 66% confidence]indpsim-models/include/dpsim-models/DP/DP_Ph3_Capacitor.h:38(details inline)
Not shown: 15 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Add mid-run variable time-step support for DP MNA solver components, including event-window refinement, documentation, and tests.
- Done: Implements Simulation/MNA solver time-step updates, adds per-component update hooks for many DP Ph1/Ph3 components, event refinement support, docs, and a new DP_VarTimeStep example test.
- Difference: none
How this review was produced
13 specialized finder passes raised 61 findings over the diff and the full changed sources. After de-duplication, 61 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 44 refuted as unsupported, 17 kept (15 tentative).
Refuted by verification:
- Clarify companion model derivation for Ph3 inductor and capacitor (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The doc explicitly says the Ph3 inductor and capacitor need no special care because their initVars() leaves the interface current alone.
- Clarify companion model derivation for Ph1 inductor and ResIndSeries (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The file already explains that DP::Ph1::Inductor and ResIndSeries preserve mIntfCurrent across initVars() because initVars() overwrites it.
- Clarify rotor angle re-anchoring for rotating machines (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The text already states the rotor angle is re-anchored by mBase_OmMech * (oldTimeStep - timeStep) and that mnaCompPreStep() advances mThetaMech.
- Clarify VBR history correction for rotating machines (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The file already says the VBR models additionally correct the history EMF for the new coefficients through adjustVBRHistoryForNewTimeStep().
- Preserve interface current in mnaUpdateTimeStep for DP Ph1 Capacitor (dpsim-models/src/DP/DP_Ph1_Capacitor.cpp): mnaUpdateTimeStep only recomputes coefficients and history terms; it never calls initVars or overwrites mIntfCurrent, and the file already preserves current in mnaCompInitialize/mnaCompUpdateCurrent patterns.
- ... 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.
|
|
||
| void initVars(Real omega, Real timeStep); | ||
| /// Reference frequency initVars() was last called with | ||
| Real mOmega = 0.0; |
There was a problem hiding this comment.
Add mTimeStep tracking to DP Ph3 Inductor
severity: medium · confidence: 68%
The DP Ph3 Inductor class does not track the last timeStep value used in initVars(), unlike DP Ph1 Inductor. This makes it impossible to detect whether a companion model re-derivation is necessary during mnaUpdateTimeStep(), leading to redundant or missed re-derivations. Tracking mTimeStep ensures companion model updates are only performed when the time step changes.
Suggested fix: Add a member variable mTimeStep to DP::Ph3::Inductor to track the last timeStep value used in initVars(). Initialize it to 0.0 in the constructor and update it in initVars() and mnaUpdateTimeStep().
Checked against the source: the class adds mOmega but contains no mTimeStep member, so mnaUpdateTimeStep cannot compare against the previous timestep
stage: model-equations
| /// init resistive companion model of capacitor | ||
| void initVars(Real omega, Real timeStep); | ||
| /// Reference frequency initVars() was last called with | ||
| Real mOmega = 0.0; |
There was a problem hiding this comment.
Add mTimeStep tracking to DP Ph3 Capacitor
severity: medium · confidence: 66%
The DP Ph3 Capacitor class does not track the last omega and timeStep values used in initVars(), unlike DP Ph1 Capacitor. This makes it impossible to detect whether a companion model re-derivation is necessary during mnaUpdateTimeStep(), leading to redundant or missed re-derivations. Tracking mOmega and mTimeStep ensures companion model updates are only performed when the time step or frequency changes.
Suggested fix: Add member variables mOmega and mTimeStep to DP::Ph3::Capacitor to track the last omega and timeStep values used in initVars(). Initialize them to 0.0 in the constructor and update them in initVars() and mnaUpdateTimeStep().
Checked against the source: mOmega is present, but the class contains no mTimeStep member to remember the timestep used for initVars
stage: model-equations
|
@FGH-wirtz thanks for the PR and the nice work! I will need a bit of time to understand and test, will get back to you with questions soon. |
…riable dt First components work with mnaUpdateTimeStep(). Resistor holds nothing that depends on the step. Inductor calls initVars(), which rebuilds the factors and the history term from the present voltage and current, but preserves mIntfCurrent across the call: initVars() overwrites it, which is right at startup where the current comes from the load flow and wrong mid-run where it is the state. Capacitor rebuilds the same way, without the overwrite that mnaCompInitialize() does. CompositePowerComp asks every sub-component and then the parent hook. The sub-result is stored before combining, because folding the call into the condition stops asking at the first component that declines and leaves the rest on the old step. mnaParentUpdateTimeStep() defaults to false, since claiming a conversion that did not happen would silence the solver warning, so composites decline until their parents are converted. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
…ariable dt Neither source stores anything derived from the step; timeStep appears only as a parameter of mnaCompInitialize(). With the passive elements from the previous commit, a network of source, R, L and C now converts completely. Testcase DP_VarTimeStep.cpp added: It changes dt mid-run on a source-R-L-C network and checks three things: changing dt to the same dt has no impact, changing from a long to a short dt gives the same result as running with the short dt from the start, and the count of components that did not follow is 0 here and 1 with a PiLine. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
…p of variable dt The "containers" hold nothing derived from step and pass it only to their sub-components, so their parent hook now converts: PiLine, RXLoad, NetworkInjection, RxLine, Transformer and Shunt, in Ph1 and Ph3 where both exist. A container still declines on its own if one of its sub-components does. The DP switches store the step as the lookahead the exponential ramp evaluates its resistance at, and take the new value. The ramp duration is in seconds and is unaffected. DP_VarTimeStep checks both answers: with a PiLine the count is 0, with a varResSwitch it is 1. varResSwitch derives its resistance step from dt and is not converted, so the count still has something to report. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
…of variable dt setEventRefinement(fine, lead, follow) uses the fine step from lead before a scheduled event until follow after it, and the step from setTimeStep() outside it. The window is evaluated at the start of each step and the change goes through updateTimeStep(), so components that cannot follow are named as before. Event times are collected when the event is added, which needs the scheduled time, so Event gets a time() accessor. DP_VarTimeStep interrupts a load branch and compares the peak node voltage during the transient against a run held at the fine step throughout: the base step alone is off by 2.9e-2, the refined window by 5.3e-8. Switching that never enters the event queue is not covered. That needs the solver to report a switch action and follows separately. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
…riable dt update Ph3 components: only PiLine, NetworkInjection and Switch were converted. Resistor, VoltageSource and CurrentSource hold nothing derived from the step. Inductor and Capacitor re-derive through initVars(), which already rebuilds the history term from the present voltage and current without touching mIntfCurrent, so they need no special treatment; initVars() now keeps the reference frequency it was called with, since a later call has only the step. DP_VarTimeStep: added a Ph3 network with a PiLine Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
… step of variable dt Ported from cw local branch, where these were already solved. ResIndSeries keeps mIntfCurrent across initVars() like the inductor. VoltageSourceNorton, ProfileVoltageSource, Inverter and the Ph3 SeriesResistor and SeriesSwitch hold nothing derived from the step. varResSwitch recomputes the resistance growth per step from the new step, the same expression setInitParameters() uses. DP_VarTimeStep swaps its unconverted component from varResSwitch, which now converts, to a ramped source. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
…le dt Ported from the cw local branch. The VBR base re-derives its constants and the resistance matrix constants, the latter because mA and mB come from the step-dependent coefficients and the stamped conductance is rebuilt from them every step. It also re-anchors mThetaMech by mBase_OmMech * (oldTimeStep - timeStep). mnaCompPreStep() advances mThetaMech to the end of the step and then passes the start time, so the transform argument carries a w0*dt term that is constant while the step is and jumps once when it changes, rotating the machine against the network. The correction carries no factor w, so the real slip against the shift frame survives. adjustVBRHistoryForNewTimeStep() undoes the coefficient swap on the history product the next stepInPerUnit() consumes; 5th order corrects the q axis only, 6a and 6b both. 4th and 6th order PCM and 4th order TPM rebuild their state space after the same re-anchoring. Ph3 DQTrapez takes the new step; DQODE declines, since the ODE solver holds its own step-dependent state. DP_VarTimeStep: added a 24 kV SMIB with a 4th order VBR machine, initialised from power flow and compares the terminal voltage after a change against a run held at the fine step: 2.3e-11. With the frame re-anchoring removed the same check reads 2.3e-1, so it measures that correction rather than passing by construction. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
… variable dt The feature was only reachable from C++. update_time_step and set_event_refinement are now bound for Python, where most runs are driven from. Adds a Developer Guide page next to the MNA solver, as an extension of it rather than a solver of its own: how a change reaches the components, what a component has to do for each of the three shapes, the base matrix that no variable component reports, the refusal on precomputed switch matrices, and the scope. Scope matters here: coverage is DP only, since mnaUpdateTimeStep() defaults to false, and refinement covers scheduled events only. Linked from the stepping section of the MNA page and, for mnaUpdateTimeStep(), from mnainterface. DP_VarTimeStep: moved from cmake "synchronous generator sources" into its own list, next to the state space examples, since it tests a solver feature. applyEventRefinement() becomes protected. Signed-off-by: cwirtz <christoph.wirtz@fgh-ma.de>
d5b0078 to
642dd32
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: The diff is broader than the stated first-stage scope: it adds support in many Ph3 components and several machine/source types not explicitly listed in the intent, and it also changes docs/navigation weights unrelated to the feature.
TL;DR: One documentation gap needs a clearer explanation of VBR history correction, and the rest of the signal is mostly low-confidence or speculative style/naming noise around noexcept and magic constants; the numerics passes did flag a few plausible parameter-validation risks in passive elements, but several are unconfirmed from the shown headers. The variable-time-step plumbing and scheduling changes otherwise did not surface any confirmed correctness or stamping/scheduling regressions in the provided findings.
Found 2 medium, 2 low (4 anchored to lines below).
🟡 Suggestions
- Magic constant without explanation
[medium · 84% confidence]indpsim-models/include/dpsim-models/DP/DP_Ph1_varResSwitch.h:66(details inline) - Incomplete description of VBR history correction
[medium · 70% confidence]indocs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md:67(details inline) - Missing SPDX header in new markdown file
[low · 95% confidence]indocs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md:1(details inline) - Magic literal used for resistance scaling factor
[low · 82% confidence]indpsim-models/include/dpsim-models/DP/DP_Ph1_varResSwitch.h:66(details inline)
Not shown: 26 tentative. All are in the workflow log.
Claim vs. implementation
- Claimed: Add variable mid-run time-step updates and scheduled-event refinement for the DP MNA solver, with component support, warnings for unsupported components, and docs/tests.
- Done: Implements mid-run time-step updates in the MNA solver and many DP Ph1/Ph3 components, plus event-window refinement and new documentation/example coverage.
- Difference: The diff is broader than the stated first-stage scope: it adds support in many Ph3 components and several machine/source types not explicitly listed in the intent, and it also changes docs/navigation weights unrelated to the feature.
How this review was produced
13 specialized finder passes raised 184 findings over the diff and the full changed sources. After de-duplication, 154 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 124 refuted as unsupported, 30 kept (26 tentative).
Refuted by verification:
- Incorrect statement about Ph3 inductor and capacitor history handling (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The file explicitly says the Ph3 inductor and capacitor need no special care because their initVars() leaves the interface current alone.
- Inconsistent description of history-term preservation across phases (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): Same passage states the claimed behavior directly, so the documentation is internally consistent on this point.
- Incomplete scope coverage for EMT and SP domains (docs/hugo/content/en/docs/Developer Guide/Solvers/variable-time-step.md): The scope section explicitly says DP only and that EMT or SP runs decline everything and warn.
- Add override specifier to mnaUpdateTimeStep in DP_Ph1_Capacitor (dpsim-models/include/dpsim-models/DP/DP_Ph1_Capacitor.h): The declaration already has
overrideon line 57. - Missing override specifier in DP_Ph1::Capacitor::mnaUpdateTimeStep implementation (dpsim-models/src/DP/DP_Ph1_Capacitor.cpp): The class declaration in DP_Ph1_Capacitor.h already declares mnaUpdateTimeStep(Real timeStep) override, so the .cpp definition does not need an override specifier.
- ... 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.
| if (timeStep <= 0) | ||
| return false; | ||
|
|
||
| mDeltaResOpen = 0.5 * timeStep / 0.001 + 1; |
There was a problem hiding this comment.
Magic constant without explanation
severity: medium · confidence: 84%
The expression 0.001 is used directly in the calculation of mDeltaResOpen inside mnaUpdateTimeStep. This literal is a magic constant that lacks documentation, violating the convention to avoid unexplained literals and to provide explanatory comments or named constants.
Suggested fix: Introduce a named constant with a descriptive name and comment its purpose, then use it in the calculation. For example, define constexpr Real RESISTANCE_GROWTH_BASE = 0.001; at the top of the file or in an appropriate namespace and replace the literal with this constant.
Checked against the source: the literal 0.001 is used directly in the resistance scaling formula without an explanation of that value
stage: naming-docs
| the step and then passes the start time, so the dq transform argument carries a `w0*dt` term. That | ||
| term is constant while the step is, and jumps once when it changes, rotating the machine against | ||
| the network. The correction takes no factor `w`, so the real slip against the shift frame | ||
| survives. The VBR models additionally correct the history EMF for the new coefficients through |
There was a problem hiding this comment.
Incomplete description of VBR history correction
severity: medium · confidence: 70%
The documentation states that VBR models correct the history EMF through adjustVBRHistoryForNewTimeStep(), but does not explain what the correction does or why it is necessary. This could leave readers unaware of the purpose of this step.
Suggested fix: Add a brief explanation of the VBR history correction: the history EMF terms depend on the companion model coefficients, which change when the time step changes. adjustVBRHistoryForNewTimeStep() adjusts the history EMF to account for the new coefficients, preserving the correct state across the step change.
Checked against the source: the VBR text only says the history EMF is corrected through the function and gives no further explanation of the correction mechanism or necessity
stage: model-equations
| @@ -0,0 +1,109 @@ | |||
| --- | |||
There was a problem hiding this comment.
Missing SPDX header in new markdown file
severity: low · confidence: 95%
The newly added documentation file does not contain an SPDX license identifier. All new source/script/workflow files must include a SPDX header using the file's comment syntax. Markdown lacks a defined comment syntax in the guidelines, but an HTML comment () should be used to satisfy the requirement.
Suggested fix: Add an HTML comment with the SPDX identifier at the top of the file, e.g., .
| --- | |
| <!-- SPDX-License-Identifier: MPL-2.0 --> | |
| --- | |
| title: "Variable Time Step in MNA" | |
| linkTitle: "Variable Time Step" | |
| ... |
Checked against the source: the new markdown file starts with Hugo front matter and contains no SPDX-License-Identifier or copyright header
stage: process-compliance
| if (timeStep <= 0) | ||
| return false; | ||
|
|
||
| mDeltaResOpen = 0.5 * timeStep / 0.001 + 1; |
There was a problem hiding this comment.
Magic literal used for resistance scaling factor
severity: low · confidence: 82%
The expression 0.5 * timeStep / 0.001 + 1 uses a hard‑coded literal 0.001 for the reference time constant. This value is not defined in Definitions.h and may diverge from other parts of the code that use a different tolerance or scaling constant.
Suggested fix: Replace the literal 0.001 with a named constant (e.g., Definitions::DEFAULT_RESISTANCE_TIME_CONSTANT) defined in Definitions.h, or expose it as a configurable parameter.
| mDeltaResOpen = 0.5 * timeStep / 0.001 + 1; | |
| mDeltaResOpen = 0.5 * timeStep / Definitions::DEFAULT_RESISTANCE_TIME_CONSTANT + 1; |
Checked against the source: mDeltaResOpen = 0.5 * timeStep / 0.001 + 1 hard-codes the reference value rather than using a named constant
stage: numerics
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #701 +/- ##
==========================================
- Coverage 65.89% 65.44% -0.46%
==========================================
Files 526 528 +2
Lines 36424 36629 +205
Branches 19438 19501 +63
==========================================
- Hits 24003 23971 -32
- Misses 12420 12657 +237
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
georgii-tishenin
left a comment
georgii-tishenin
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR. From my point of view looks good. I have added two optional comments (if you do not agree with them, just resolve). Same goes for the comments from the GitHub actions bot.
Let's also wait for the review from @leonardocarreras
| SPDLOG_LOGGER_WARN(mSLog, | ||
| "Time step changed while using precomputed switch " | ||
| "matrices. They keep the factorisation of the old step; " | ||
| "enable system matrix recomputation to change it."); |
There was a problem hiding this comment.
I suggest throwing an exception with clear message (maybe same as you wrote) rather than logging a warning. This is clearly a wrong combination of settings. The warning can be overlooked by the user.
| } | ||
|
|
||
| template <typename VarType> | ||
| void MnaSolver<VarType>::resolveSystemMatrixRecomputationMode() { |
There was a problem hiding this comment.
in one of the recent PRs we have added this logic to automatically decide whether recomputation should be enabled or not based on the components in the topology. Could variable timestep be integrated into the existing SystemMatrixRecomputationMode::Auto logic?
If event refinement / variable timestep is enabled before initialization, Auto could select system-matrix recomputation automatically, because timestep-dependent companion stamps need to be rebuilt when dt changes. If the user explicitly selects SystemMatrixRecomputationMode::Disabled together with variable timestep, I would prefer throwing a SystemError instead of warning, since continuing with a factorization from the old timestep is not a valid configuration.
For manual updateTimeStep() calls after start, the solver should probably still throw if the current matrix mode cannot support timestep changes.
3 participants
First stage of #680. There i proposed implementing an event-driven variable step so a run can use a large step in the settled state and a fine step after a fault. This branch starts with DP domain and scheduled events.
What it adds
setTimeStep() only works before a run starts, because components build their companion models from the step at initialisation and keep them. New: Simulation::updateTimeStep(Real) changes the step properly between steps, and setEventRefinement(fine, lead, follow) uses a finer step around scheduled events.
A change travels Simulation::updateTimeStep() → MnaSolver::updateTimeStep() → MNAInterface::mnaUpdateTimeStep() per component. A component returns false if it cannot follow; the solver counts those, logs the declining types and returns the count, so a stale discretisation is reported rather than silently simulated.
Two solver details:
Coverage and limits
All of DP::Ph1 and DP::Ph3: passive elements, ideal sources, containers, switches, and the reduced-order and PCM/TPM machines. Machines also re-anchor the rotor angle by mBase_OmMech * (oldTimeStep - timeStep), otherwise the dq transform argument jumps by w0*dt and the machine rotates against the network.
mnaUpdateTimeStep() defaults to false, so EMT and SP decline and warn, and existing runs are untouched. Still declining in DP on purpose: the converter families, SynchronGeneratorTrStab, SynchronGeneratorIdeal, VoltageSourceRamp, Ph3::SynchronGeneratorDQODE.
Refinement covers scheduled events only for now. Important: Logs get non-uniform time stamps, so anything downstream has to read the time column instead of assuming a constant step.
Testing
DP_VarTimeStep in dpsim/examples/cxx/Components/, six checks:
nothing changes on an unchanged step 0
coarse-to-fine against a run held fine 4.4e-10
decline count 0 passive and 1 with a ramped source
refined window on a switching transient 2.9e-2 at the base step against
5.3e-8 refined;
machine rotor angle 2.3e-11.
The machine case is a 24 kV SMIB with a 4th-order VBR generator. Remove the rotor re-anchoring and the same check reads 2.3e-1, so it measures that correction rather than passing by construction.
Documented under Developer Guide, Solvers, next to the MNA solver page.
An LLM was used to rework my local implementation for this merge request - i verified changes and modified some things manually.