feat: configurable PF solver Newton step clamp - #559
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
==========================================
- Coverage 72.14% 72.07% -0.07%
==========================================
Files 491 491
Lines 31623 31645 +22
Branches 16950 16960 +10
==========================================
- Hits 22815 22809 -6
- Misses 8722 8811 +89
+ Partials 86 25 -61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
40215a8 to
9e4382c
Compare
![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
TL;DR: One real issue remains: the new PF step-clamp setter accepts invalid values without validation, which can invert or break Newton step scaling and cause divergence; the rest are lower-value documentation/naming notes, and the equation/stamping path otherwise looks consistent with the PR intent.
Found 1 high, 2 medium (2 anchored to lines below).
🔴 Critical & high
- Missing validation for Newton step clamp parameters
[high · 90% confidence]indpsim/include/dpsim/PFSolver.h:233(details inline)
🟡 Suggestions
- Python binding lacks parameter validation documentation
[medium · 62% confidence]indpsim/src/pybind/main.cpp:235(details inline)
🔵 Optional / low-confidence (1)
- Clarify comment for mPFMaxDVoltagePerUnitPerStep and mPFMaxDThetaRadPerStep
[medium · 35% confidence · unconfirmed]indpsim/include/dpsim/Simulation.h:53
How this review was produced
13 specialized finder passes raised 15 findings over the diff and the full changed sources. After de-duplication, 15 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 9 refuted as unsupported, 6 kept (1 tentative).
Refuted by verification:
- Parameter validation missing for setPFSolverMaxStepPerIteration (dpsim/src/Simulation.cpp): The setter is a direct attribute assignment and the same pattern is used by nearby setters like setPFBaseApparentPowerFallback and setPFMaxIterations; no validation exists in the file to be missing here.
- Missing input validation in Simulation::setPFSolverMaxStepPerIteration (dpsim/src/Simulation.cpp): Simulation::createSolvers() forwards the stored values to pfSolver->setMaxStepPerIteration() at lines 145-146, so the setter is not the only path and the file shows the intended propagation.
- Use DOUBLE_EPSILON or a defined constant for default step clamp values (dpsim/include/dpsim/PFSolver.h): the clamp defaults are explicit documented domain limits, not epsilon tolerances, and no provided code establishes a required constant for these values
- Use DOUBLE_EPSILON or a defined constant for step clamp comparison (dpsim/src/PFSolverPowerPolar.cpp): updateSolution now reads configurable member variables rather than hard-coded literals at the comparison/clamp site
- Clarify comment for mMaxDVoltagePerUnitPerStep and mMaxDThetaRadPerStep (dpsim/include/dpsim/PFSolver.h): the comment already explicitly names the derived PFSolverPowerPolar::updateSolution() method that uses the base members
- Clarify comment for step scaling in updateSolution() (dpsim/src/PFSolverPowerPolar.cpp): the comment correctly describes the scaling and is immediately followed by the configurable member-derived limits
- Clarify Python binding names for PF solver Newton step clamp methods (dpsim/src/pybind/main.cpp): the exposed Python names are lowercase underscore-separated names, not camelCase, and match the surrounding pf solver binding style
- Missing Python getter for PFSolverMaxStepPerIteration (dpsim/src/pybind/main.cpp): both individual getters are bound, and no combined tuple getter requirement is evidenced by this file or related sources
- Add documentation for new PF solver step-clamp members (dpsim/include/dpsim/PFSolver.h): the members have inline comments giving purpose and units, and the setter comment documents the default values
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.
| CPS::UInt getMaxIterations() const { return mMaxIterations; } | ||
|
|
||
| /// Relax or tighten the per-iteration Newton step clamp (defaults: 0.1 pu / 0.2 rad). | ||
| void setMaxStepPerIteration(CPS::Real maxDVoltagePerUnit, |
There was a problem hiding this comment.
Missing validation for Newton step clamp parameters
severity: high · confidence: 90%
The setter PFSolver::setMaxStepPerIteration accepts arbitrary real values for maxDVoltagePerUnit and maxDThetaRad. If a negative or zero value is passed, the Newton‑Raphson step scaling in PFSolverPowerPolar::updateSolution() will compute a negative scale factor, inverting the direction of the update and potentially causing divergence or physically meaningless voltage/angle updates. The original hard‑coded defaults were positive, so this regression can introduce incorrect convergence behavior when the API is misused.
Suggested fix: Validate the input parameters to ensure they are strictly positive before storing them. Throw an exception or clamp to a minimal positive value if the check fails.
| void setMaxStepPerIteration(CPS::Real maxDVoltagePerUnit, | |
| void setMaxStepPerIteration(CPS::Real maxDVoltagePerUnit, | |
| CPS::Real maxDThetaRad) { | |
| if (CPS::Math::isPositiveFinite(maxDVoltagePerUnit) && | |
| CPS::Math::isPositiveFinite(maxDThetaRad)) { | |
| mMaxDVoltagePerUnitPerStep = maxDVoltagePerUnit; | |
| mMaxDThetaRadPerStep = maxDThetaRad; | |
| } else { | |
| SPDLOG_LOGGER_ERROR(mSLog, | |
| "Invalid Newton step clamp values: maxDVoltagePerUnit={} maxDThetaRad={}. " | |
| "Using defaults (0.1 pu / 0.2 rad).", | |
| maxDVoltagePerUnit, maxDThetaRad); | |
| } | |
| } |
Checked against the source: negative clamp arguments are accepted unmodified, which can make updateSolution compute a negative scale factor
stage: model-equations
| &DPsim::Simulation::getPFBaseApparentPowerFallback) | ||
| .def("set_pf_max_iterations", &DPsim::Simulation::setPFMaxIterations) | ||
| .def("get_pf_max_iterations", &DPsim::Simulation::getPFMaxIterations) | ||
| .def("set_pf_solver_max_step_per_iteration", |
There was a problem hiding this comment.
Python binding lacks parameter validation documentation
severity: medium · confidence: 62%
The Python binding exposes set_pf_solver_max_step_per_iteration but does not document the expected range or units for the arguments. Users may pass invalid values (e.g., negative or NaN) without realizing it, leading to silent failures or incorrect results.
Suggested fix: Add a docstring to the Python binding that specifies the expected units (pu for voltage, radians for angle), the valid range (strictly positive), and the default values. Example: 'maxDVoltagePerUnit (float): Maximum per-iteration voltage step magnitude in per unit. Must be > 0. Default 0.1. maxDThetaRad (float): Maximum per-iteration angle step magnitude in radians. Must be > 0. Default 0.2.'
Checked against the source: the new pybind setter is exposed without py::arg names or a docstring describing units or valid ranges
stage: numerics
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: No correctness issues were found in the solver logic itself; the only actionable gaps are missing docs and test coverage for the new configurable PF Newton step clamp API, so the main risk is an unverified and undiscoverable feature rather than a functional regression.
Found 4 medium (0 anchored to lines below).
🔵 Optional / low-confidence (4)
- Add documentation for PFSolver Newton step clamp configuration
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Concepts/PFSolver.md - Add a pytest or notebook test exercising the new PF solver step-clamp API
[medium · 35% confidence · unconfirmed]intests/notebooks - Missing documentation for configurable PF solver Newton step clamp
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Concepts - Missing test for new PF solver step‑clamp configuration
[medium · 35% confidence · unconfirmed]intests/python
Claim vs. implementation
- Claimed: Make the PF solver Newton step clamp configurable via Simulation/PFSolver and pybind, with unchanged defaults.
- Done: Adds configurable max per-iteration voltage/angle step limits to PFSolver, wires them through Simulation and Python bindings, and keeps defaults at 0.1 pu / 0.2 rad.
- Difference: none
How this review was produced
13 specialized finder passes raised 26 findings over the diff and the full changed sources. After de-duplication, 26 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 22 refuted as unsupported, 4 kept (4 tentative).
Refuted by verification:
- Incorrect validation of Newton step-clamp limits (dpsim/include/dpsim/PFSolver.h): The check uses <= 0 and the comment explicitly says zero stalls and negative diverges, so rejecting non-positive values is intentional.
- Reject zero Newton step-clamp limits to avoid solver stall (dpsim/include/dpsim/PFSolver.h): The method already rejects zero with <= 0, matching the stated clamp semantics.
- Reject zero Newton step-clamp limits explicitly (dpsim/include/dpsim/PFSolver.h): The claim about NaN/infinity is not supported by the file; the visible validation is for non-positive values only.
- Document units and rationale for step-clamp limits (dpsim/include/dpsim/PFSolver.h): The comment already states the units as [pu] and [rad] and explains zero/negative behavior.
- Attribute usage: mPFMaxDVoltagePerUnitPerStep and mPFMaxDThetaRadPerStep are static and should be AttributeStatic (dpsim/include/dpsim/Simulation.h): The members are intentionally declared as const CPS::Attribute::Ptr and the file already uses AttributeStatic elsewhere as a normal DPsim pattern, so this is not a defect in the shown code.
- Reject zero or negative Newton step-clamp limits in setMaxStepPerIteration (dpsim/include/dpsim/PFSolver.h): The exception message is a fixed validation message by design; lack of echoed values is not a defect in the code shown.
- Add virtual destructor to polymorphic base PFSolver (dpsim/include/dpsim/PFSolver.h): PFSolver already declares a virtual destructor:
virtual ~PFSolver(){}. - Add virtual destructor to polymorphic Simulation (dpsim/include/dpsim/Simulation.h): Simulation already declares a virtual destructor at line 194: virtual ~Simulation() {}.
- Clarify comment on setMaxStepPerIteration (dpsim/include/dpsim/PFSolver.h): The comment says it relaxes or tightens the clamp, which matches a setter for the clamp limits.
- Clarify comment on setPFSolverMaxStepPerIteration (dpsim/include/dpsim/Simulation.h): The comment is present directly above setPFSolverMaxStepPerIteration and matches the new API added in this file.
- Remove embedded newlines from log message header (dpsim/src/PFSolverPowerPolar.cpp): The header is a single INFO log call at line 350; there is no embedded newline in the message string.
- Expose Simulation::getPFSolverMaxStepPerIteration to Python (dpsim/src/pybind/main.cpp): The getter is already bound on line 239 as get_pf_solver_max_dtheta_rad_per_step to Simulation::getPFSolverMaxDThetaRadPerStep.
- Missing input validation for step-clamp limits in setMaxStepPerIteration (dpsim/include/dpsim/PFSolver.h): The assignments occur only after the validation and throw, so invalid values do not overwrite the members.
- Add missing include for std::invalid_argument (dpsim/include/dpsim/PFSolver.h): is included at line 13, so std::invalid_argument is available.
- Add missing SPDX header to new include line (dpsim/include/dpsim/PFSolver.h): The file already has a copyright/MPL header at the top; no SPDX header is required by the shown source.
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.
1 participant
Summary