IEEEST Stabilizer Improvements/Corrections by lukelowry · Pull Request #460 · ORNL/GridKit

lukelowry · 2026-06-22T16:49:18Z

Description

Fixes and cleans up the IEEEST stabilizer implementation and documentation.

This updates the older initial implementation style so that reduced-order notch filters and zero denominator time constants are handled consistently

Proposed changes

Replace the branchy/safe-inverse notch logic with a fixed DAE form using differential tags.
Initialize IEEEST from the attached input signal instead of forcing all states to zero.
Document the implemented equations, initialization, derived parameters, and unsupported compatibility parameters.
Tests for initialization, residuals, invalid wiring, invalid notch parameters, zero time constants, and Jacobian consistency.

Checklist

All tests pass for the focused PhasorDynamics coverage.
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.

Changelog changes N/A

Further comments

This is cleanup/correction work for the existing IEEEST model, not a new model

lukelowry · 2026-06-22T18:44:31Z

@pelesh this fix is also ready to be merged I forgot to make a PR for this one.

pelesh

The logic governing changing differential into algebraic equations and vice versa does not seem to be sustainable. This requires more offline discussion.

lukelowry · 2026-06-26T12:48:23Z

I have rebased and adjusted the formulation to align with #463 .

nkoukpaizan

I have a few comments here. Perhaps worth discussing at our next meeting.

nkoukpaizan · 2026-06-28T22:26:09Z

+        const auto U   = static_cast<size_t>(IeeestExternalVariables::U);
+
+        const ScalarT x1  = y[X1];
+        const ScalarT x2  = y[X2];


Same here, the indirection from the enum is less readable to me. We should discuss a sustainable way to use the enums.

nkoukpaizan · 2026-06-28T22:39:34Z

        using Variable = typename ModelDataT::MonitorableVariables;
-        monitor_->set(Variable::vss, [this]
-                      { return y_[11]; });
+        auto index     = [](IeeestInternalVariables variable)


I've seen this pattern a few times. Consider defining once if it is really needed. I would argue it's less readable.

lukelowry · 2026-07-02T15:43:03Z

Testing Ground for template parameters <Order N>

lukelowry · 2026-07-08T22:25:20Z

general implementation of the template parameter done

lukelowry · 2026-07-20T15:13:53Z

@nkoukpaizan what are your thoughts on my template implementation?

nkoukpaizan · 2026-07-21T17:43:21Z

@nkoukpaizan what are your thoughts on my template implementation?

The model implementation is cleaner with the order as a template parameter. However, the order remains a runtime selection based on floating point comparisons, which is what I wanted to avoid.

lukelowry · 2026-07-21T18:40:44Z

@nkoukpaizan we could possibly require the order to be passed as a parameter of IEEEST model, would that resolve your concern?

"Order" is not usually directly exposed as a model parameter and so would require some knowledge of how IEEEST works to pass valid parameters.

Can you give an example where the runtime comparison is bad/fails? I might be able to resolve both our concerns if I understand better.

nkoukpaizan · 2026-07-21T19:03:05Z

@nkoukpaizan we could possibly require the order to be passed as a parameter of IEEEST model, would that resolve your concern?

"Order" is not usually directly exposed as a model parameter and so would require some knowledge of how IEEEST works to pass valid parameters.

Can you give an example where the runtime comparison is bad/fails? I might be able to resolve both our concerns if I understand better.

Yes, exposing the order as a parameter to the model is what I had in mind. Understood that that's not the domain practice. That's why as was asking whether this is possible as a question.

Floating point values are stored with limited precision. We'd have to guarantee that the value returned by the parser is bit-for-bit equal to zero. That may be the case for nlohmann-json, but considering #227 and the possibility of using a different parser, that's not guaranteed.

lukelowry · 2026-07-22T05:58:40Z

@nkoukpaizan i see the issue now thank you, that was very clarifying. After reflecting on my past experience and discussing with others at TAMU, I think deriving the order from the parameters alone is not intuitive or safe, either.

As discussed, we should add an integer parameter to the IEEEST model, "order" and use that for the StabilizerFactory (rename to IeeestFactory). Then, our parameter validation can gate for zero division relevant to each static order (e.g. $A_2 + A_3>0$). Additionally the model json may then contain some coefficients that are only used at a certain "order", which would be a feature IMO since you can retain information needed for higher order but choose to operate at smaller order.

I will make this change then we can reasess.

lukelowry · 2026-07-22T22:34:41Z

@nkoukpaizan I have made the proposed edits. For future models with many different "orders" for subsystems the template arguments might get long, but it works well for one argument here

nkoukpaizan

The template for the stabilizer object looks a lot cleaner to me!

I'm having a hard time reading the StabilizerIeeestTests at the moment, especially where multiple cases are defined as vectors of arrays. I would split the cases into separate tests and implement specific, reusable private methods.

nkoukpaizan · 2026-07-23T20:18:14Z

+      /// (a1 = a3 = 0)
+      ///
+      /// The Enzyme-vs-dependency-tracking Jacobian comparison is not run for
+      /// this configuration: Enzyme's `sparse_store` drops exact-zero entries


Enzyme should not drop non-structural zeros. May be worth looking into it if the Jacobians differ. I recommend starting with an implementation of the test with an expectFailure outcome.

nkoukpaizan · 2026-07-23T20:22:33Z

-        int ret = 0;
-
-        if (signals_.template isAttached<IeeestExternalVariables::U>())
+        if (value != ZERO<RealT>)


I think isEqual would be appropriate here. I recommend a PR that moves it from the Testing namespace to, e.g., Math. @lukelowry Let me know if you can do that or if you'd rather me to.

nkoukpaizan · 2026-07-24T14:25:36Z

-       * @brief Compare DependencyTracking Jacobian against Enzyme Jacobian.
-       */
-      TestOutcome jacobian()
+      TestOutcome verify()


Verify currently logs some errors. Make sure to message the user about expected errors, similar to #500.

nkoukpaizan · 2026-07-24T14:28:58Z

-  result += test.zeroInitialResidual();
-  result += test.residual();
+
+  result += test.init<0>();


It might be cleaner to template the test class rather than individual member functions.

If I understand correctly, some tests are manipulating stabilizer objects of different orders. Consider two test objects:

One templated to check all orders

Another one that manipulates/compares objects of different orders.

nkoukpaizan · 2026-07-24T14:31:51Z

@@ -19,10 +19,11 @@ namespace GridKit
       *
       * @tparam ScalarT - Scalar data type
       * @tparam IdxT    - Index data type


Note the changes in templating style in #410 and #505. Something to be careful with when rebasing. We'll have to make sure new instances of the old format are not introduced in new PRs.

nkoukpaizan · 2026-07-24T14:36:01Z

+        using Params = PhasorDynamics::Stabilizer::IeeestParameters;
+
+        // The second case exercises the TIME_CONSTANT_MINIMUM flooring.
+        const std::vector<std::array<RealT, 3>> time_constant_cases = {


I would split into two different tests.

lukelowry marked this pull request as ready for review June 22, 2026 18:43

lukelowry mentioned this pull request Jun 23, 2026

10k Bus Case #461

Draft

7 tasks

lukelowry force-pushed the lukel/ieeest-dev branch from 509506c to e2c07d8 Compare June 24, 2026 20:00

pelesh requested changes Jun 24, 2026

View reviewed changes

Comment thread GridKit/Model/PhasorDynamics/Stabilizer/IEEEST/IeeestImpl.hpp Outdated

Comment thread GridKit/Model/PhasorDynamics/Stabilizer/IEEEST/IeeestImpl.hpp Outdated

Comment thread GridKit/Model/PhasorDynamics/Stabilizer/IEEEST/IeeestImpl.hpp Outdated

lukelowry force-pushed the lukel/ieeest-dev branch from e2c07d8 to bab57ad Compare June 24, 2026 22:11

pelesh mentioned this pull request Jun 25, 2026

Make PhasorDynamics models Hessenberg-ready #463

Open

lukelowry force-pushed the lukel/ieeest-dev branch 2 times, most recently from 8bbbad6 to 090289b Compare June 26, 2026 11:02

lukelowry force-pushed the lukel/ieeest-dev branch from 755ee20 to 282fc1d Compare June 26, 2026 21:59

nkoukpaizan reviewed Jun 28, 2026

View reviewed changes

lukelowry force-pushed the lukel/ieeest-dev branch from 282fc1d to 579b3c1 Compare July 1, 2026 05:17

lukelowry added this to the WECC Completion milestone Jul 2, 2026

lukelowry force-pushed the lukel/ieeest-dev branch from 579b3c1 to 9fb15fa Compare July 3, 2026 00:25

lukelowry force-pushed the lukel/ieeest-dev branch from 7336d1f to 1104b91 Compare July 9, 2026 16:09

nkoukpaizan added enhancement New feature or request cleanup labels Jul 10, 2026

lukelowry force-pushed the lukel/ieeest-dev branch 4 times, most recently from d0dc95a to d2be200 Compare July 17, 2026 21:58

lukelowry force-pushed the lukel/ieeest-dev branch from d2be200 to 47a31f1 Compare July 22, 2026 06:46

lukelowry force-pushed the lukel/ieeest-dev branch from 47a31f1 to 5205fbe Compare July 22, 2026 14:59

nkoukpaizan reviewed Jul 24, 2026

View reviewed changes

lukelowry added 9 commits July 24, 2026 18:15

Fix bad parameter initialization and polish docs

1cc2b37

Update Docs

de299dd

isEqual change and tidy

141c08d

Hessenberg form

e11e4c5

rebased IEEEST

a6e8c4c

tempalte parameter for order implemetnations

006dc11

clean/correct ieeest impl per review

54b30da

IeeestFactory rename

f78e899

Add order as parameter

fe235d0

lukelowry force-pushed the lukel/ieeest-dev branch from b33b975 to fe235d0 Compare July 24, 2026 23:18

Uh oh!

Conversation

lukelowry commented Jun 22, 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!

lukelowry commented Jun 22, 2026

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukelowry commented Jun 26, 2026

Uh oh!

nkoukpaizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukelowry commented Jul 2, 2026

Uh oh!

lukelowry commented Jul 8, 2026

Uh oh!

lukelowry commented Jul 20, 2026

Uh oh!

nkoukpaizan commented Jul 21, 2026

Uh oh!

lukelowry commented Jul 21, 2026

Uh oh!

nkoukpaizan commented Jul 21, 2026

Uh oh!

lukelowry commented Jul 22, 2026

Uh oh!

lukelowry commented Jul 22, 2026

Uh oh!

nkoukpaizan 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukelowry commented Jun 22, 2026 •

edited

Loading