Model adjustments caught during validation by lukelowry · Pull Request #525 · ORNL/GridKit

lukelowry · 2026-08-05T22:45:24Z

Description

These are the last few issues I ran into during validation. Mostly initialization and parameter guard fixes.

Lots of tiny edge cases in the full unmodified cases that we did not account for.

Proposed changes

Misc parameter gaurds
Misc formatting while I was touching files
Simplified & fixed earlier models that existed before the newer API

Checklist

All tests pass.
Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
The new code follows GridKit™ style guidelines.
There are unit tests for the new code.
The new code is documented.
The feature branch is rebased with respect to the target branch.
NA I have updated CHANGELOG.md to reflect the changes in this PR. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

Looking very good. I think you all will be very happy with the validation results. Sneak peek:

abirchfield

These are good cleanup steps that help clarify documentation, increase test coverage, and update a few implementation details.

pelesh

Great work and very helpful expansion of unit tests.

There are some solutions in this PR that I don't quite understand (see comments below) and I would like us to clarify that before we merge.

pelesh · 2026-08-12T17:59:01Z

+          Ispdlim_ = std::visit([](auto value)
+                                { return value != 0 ? ONE<RealT> : ZERO<RealT>; },
+                                data.parameters.at(Parameter::Ispdlim));


It is difficult to see what this function does. I suggest rewriting or at least commenting better.

The rest of the code below is quite cryptic. It needs to be commented better.

pelesh · 2026-08-12T18:05:00Z

+When one saturation value is zero, the normal curve fit uses the corresponding
+voltage as the quadratic knee:
+


I believe you wanted to say "When only one ...". Also, it is not quite clear to me what the rest of this sentence means.

pelesh · 2026-08-12T18:23:59Z

+        if (Pvmax_ < Pvmin_)
+        {
+          std::swap(Pvmin_, Pvmax_);
+        }


I don't know what is the purpose of this but I don't see anything good coming from swapping Pvmax and Pvmin.

Commercial software often has a practice of trying to determine user intent when bad data is encountered. In this case, the most likely explanation for Pvmax < Pvmin is that the user accidentally switched them. Some users prefer this autocorrection to throwing lots of errors, especially when they don't have detailed familiarity with the models. I'm OK with this or without it.

pelesh · 2026-08-12T18:32:50Z

+      if (H_ == ZERO<RealT> && D_ == ZERO<RealT>)
+      {
+        H_inv_ = ZERO<RealT>;
+      }
+      else
+      {
+        H_     = std::max(H_, static_cast<RealT>(0.1));
+        H_inv_ = ONE<RealT> / (TWO<RealT> * H_);
+      }


This is in my opinion unnecessary and is a bad modeling practice. Setting H_inv_ to zero changes the model.

Sometimes there is a convention of letting a 0 value represent infinity, for a parameter for which 0 does not make sense. A GENCLS with infinite inertia is just an infinite bus, and this is sometimes used to represent infinite buses.

pelesh · 2026-08-12T18:39:24Z

+      H_ = std::max(H_, static_cast<RealT>(0.1));
+      if (Xdp_ > Xd_)
+      {
+        Xdp_ = static_cast<RealT>(0.8) * Xd_;
+      }
+      if (Xqp_ > Xq_)
+      {
+        Xqp_ = Xq_;
+      }
+      const RealT transient_min = std::min(Xdp_, Xqp_);
+      if (Xdpp_ > transient_min)
+      {
+        Xdpp_ = static_cast<RealT>(0.8) * transient_min;
+      }
+      Xdpp_ = std::max(Xdpp_, static_cast<RealT>(0.05));
+      if (Xl_ > Xdpp_)
+      {
+        Xl_ = static_cast<RealT>(0.8) * Xdpp_;
+      }
+      Xqpp_ = Xdpp_;


Changing user input silently is a very bad idea. There has to be justification for that, and this block of code needs comments explaining what is done here and why.

I think these are the PowerWorld autocorrection rules, trying to fix obviously bad data for a user who may not have the expertise to determine what values should be put for these parameters. I guess there is a general question of whether we should implement autocorrection or not.

These are the autocorrection rules from PowerWorld. I will add a logger warning when this autocorrection occurs.

I think this is necessary if we are going to be validating against PowerWorld. If we want to get to solver-tolerance precision against a reference solution, I need to be certain it's not autocorrected parameters causing differences. I also agree with @abirchfield, for his listed reasons

pelesh · 2026-08-12T18:42:46Z

  0 &= -V_{d} -\psi''_{q}(1+\omega)\\
  0 &= -V_{q}  +\psi''_{d}(1+\omega)\\
  0 &= -T_{elec} +(\psi''_{d} - I_dX_d'')I_q-(\psi''_{q} - I_qX_d'')I_d \\
-  0 &= -k_{sat} + S_B(\psi''-S_A)^2\sigma(\psi''-S_A) \\
+  0 &= -k_{sat} + S_B q(\psi''-S_A) \\
  0 &= -I_d + I_r \sin(\delta) - I_i \cos(\delta) \\
  0 &= -I_q + I_r \cos(\delta) + I_i \sin(\delta) \\
  0 &= -I_r + G (V_d \sin(\delta) + V_q \cos(\delta) - V_r) - B (V_d \cos(\delta) + V_q \sin(\delta) - V_i) \\
  0 &= -I_i + B (V_d \sin(\delta) + V_q \cos(\delta) - V_r) + G (V_d \cos(\delta) + V_q \sin(\delta) - V_i)


These equations do not render correctly.

pelesh · 2026-08-12T18:46:40Z

+      H_ = std::max(H_, static_cast<RealT>(0.1));
+      if (Xdp_ > Xd_)
+      {
+        Xdp_ = static_cast<RealT>(0.8) * Xd_;
+      }
+      if (Xdpp_ > Xdp_)
+      {
+        Xdpp_ = static_cast<RealT>(0.8) * Xdp_;
+      }
+      Xdpp_ = std::max(Xdpp_, static_cast<RealT>(0.05));
+      if (Xl_ > Xdpp_)
+      {
+        Xl_ = static_cast<RealT>(0.8) * Xdpp_;
+      }


Again, changing user's input silently is a Bad Idea™.

pelesh · 2026-08-12T18:51:06Z

+      static constexpr ScalarT tol_ = 100 * std::numeric_limits<ScalarT>::epsilon();



Why are we using 100 x epsilon tolerance? It would be good to have this value set as global testing requirement rather than set it on a test by test basis.

pelesh · 2026-08-12T20:01:26Z

+        Ke_     = ke0;
        y[EFDP] = efdp0;


Here is another instance of auto-correction. It is not commented and it is unclear why user supplied parameter is changed.

pelesh · 2026-08-12T20:02:05Z

+        RealT ke0 = Ke_;
+        if (ke0 == ZERO<RealT>)


Another autocorrection here.

pelesh · 2026-08-12T20:28:31Z

+        Pvmin_ = std::min(Pvmin_, static_cast<RealT>(pv0));
+        Pvmax_ = std::max(Pvmax_, static_cast<RealT>(pv0));


I think this is really dangerous auto-correct. Instead of changing initial value, it changes properties of the governor.

This one is actually the intended behavior of the governor. If we solved for the steady-state power flow solution in the equivalent PowerFlow or OPF model, we would set the constraint $P_{vmin} \leq P_{gen} \leq P_{vmax}$, and this initialization-stage autocorrection would never be invoked in an ideal situation. However, if the models' PowerFlow data and PhasorDynamics data are different, then it would be possible for the power flow solution to be an infeasible steady-state solution in the transient model.

I'll add a logger warning for this. A lot of the test cases have a power flow solution that invokes this.

lukelowry force-pushed the lukel/param-fixes-dev branch 2 times, most recently from 80cc441 to 1bff371 Compare August 8, 2026 04:26

lukelowry marked this pull request as ready for review August 8, 2026 11:32

lukelowry mentioned this pull request Aug 8, 2026

10k Bus Case + Three Fully Validated Cases #461

Open

6 tasks

lukelowry force-pushed the lukel/param-fixes-dev branch from 1bff371 to b925f6b Compare August 8, 2026 19:32

abirchfield approved these changes Aug 11, 2026

View reviewed changes

lukelowry added 4 commits August 11, 2026 16:08

Final corrections of old models

9f96d0a

Misc parameter vlaidation corrections

56017fa

tests and documentation and impl in agreement

a5a1eea

minor doc notes

4eb2d17

lukelowry force-pushed the lukel/param-fixes-dev branch from b925f6b to 4eb2d17 Compare August 11, 2026 21:31

lukelowry added this to the Release 0.2 milestone Aug 11, 2026

pelesh requested review from nkoukpaizan and pelesh August 12, 2026 17:48

pelesh requested changes Aug 12, 2026

View reviewed changes

pelesh reviewed Aug 12, 2026

View reviewed changes

lukelowry added 3 commits August 12, 2026 19:11

add logger warnings

250f8ef

verify ieeet1 call

5027b69

misc additions

b47479a

nkoukpaizan assigned lukelowry Aug 13, 2026

nkoukpaizan added bug Something isn't working cleanup labels Aug 13, 2026

lukelowry marked this pull request as draft August 13, 2026 20:44

nkoukpaizan mentioned this pull request Aug 13, 2026

Minimal corrections to Phasor Dynamics models for validation #538

Open

lukelowry closed this Aug 14, 2026

		When one saturation value is zero, the normal curve fit uses the corresponding
		voltage as the quadratic knee:

		static constexpr ScalarT tol_ = 100 * std::numeric_limits<ScalarT>::epsilon();

		Pvmin_ = std::min(Pvmin_, static_cast<RealT>(pv0));
		Pvmax_ = std::max(Pvmax_, static_cast<RealT>(pv0));

Uh oh!

Conversation

lukelowry commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed changes

Checklist

Further comments

Uh oh!

abirchfield left a comment

Choose a reason for hiding this comment

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukelowry commented Aug 5, 2026 •

edited

Loading