Initial lambda extraction by lukelowry · Pull Request #566 · ORNL/GridKit

lukelowry · 2026-09-02T06:06:06Z

Description

Extract the inconsistent lambdas for parameter validation and signal attachment into one common layer

Proposed changes

Utilities::ConfigurationCheck
Utilities::ParameterReader

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 The CHANGELOG.md has been updated to reflect the changes. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

@nkoukpaizan Drafted until you take a quick look, is this good approach generally?

lukelowry · 2026-09-10T00:13:44Z

I am marking this as ready for review, so that we can determine remaining priorities before release.

Let me know if this is worth it before release

nkoukpaizan

I like the idea of extracting the lambda. What we need to discuss is where the extracted methods will live.

nkoukpaizan · 2026-09-08T18:07:43Z

      std::unique_ptr<MonitorT> monitor_;
+
+      /// Count of parameter-loading errors reported through verify()
+      IdxT parameter_error_count_{0};


I wouldn't think we need to store this as a member variable. Removing would also avoid the static_cast<int>.

nkoukpaizan · 2026-09-08T18:13:26Z

+      template <ExternalVariables variable>
+      auto refreshWorkspace(ScalarT fallback, ScalarT* ws, IdxT* ws_indices) const
+      {
+        static_assert(variable < ExternalVariables::MAXIMUM);
+        const auto index  = static_cast<size_t>(variable);
+        ws[index]         = fallback;
+        ws_indices[index] = INVALID_INDEX<IdxT>;
+        if (isAttached<variable>())
+        {
+          ws[index]         = readExternalVariable<variable>();
+          ws_indices[index] = readExternalVariableIndex<variable>();
+        }
+      }


I wouldn't make ComponentSignals depend on ws_. Let's keep these in *Impl.hpp.

nkoukpaizan · 2026-09-08T18:17:45Z

+      template <ExternalVariables variable>
+      auto checkRequired(Utilities::ConfigurationChecks& checks, const char* name) const
+      {
+        static_assert(variable < ExternalVariables::MAXIMUM);
+        if (!isAttached<variable>())
+        {
+          checks.fail() << name << " signal is required\n";
+          return;
+        }
+        if (!isLinked<variable>())
+        {
+          checks.fail() << name << " signal attached with no linked source\n";
+        }
+      }
+
+      /// Verifies a port that may be absent but must resolve to linked
+      /// storage when attached
+      ///
+      /// @tparam variable The external variable to verify
+      /// @param[in,out] checks Error accumulator for the owning component
+      /// @param[in] name Port name used in error messages
+      template <ExternalVariables variable>
+      auto checkOptional(Utilities::ConfigurationChecks& checks, const char* name) const
+      {
+        static_assert(variable < ExternalVariables::MAXIMUM);
+        if (!isAttached<variable>())
+        {
+          return;
+        }
+        if (!isLinked<variable>())
+        {
+          checks.fail() << name << " signal attached with no linked source\n";
+        }
+      }


Let's not make ComponentSignals depend on ConfigurationChecks. Let's keep these in *Impl.hpp.

nkoukpaizan · 2026-09-08T18:18:18Z

+/**
+ * @file ConfigurationChecks.hpp
+ * @author Luke Lowery (lukel@tamu.edu)
+ * @brief Error accumulator for model configuration validation.


Documentation will need an update.

nkoukpaizan · 2026-09-10T14:11:06Z

+     *         the `Parameters` enumeration, and the `parameters` map.
+     */
+    template <typename ModelDataT>
+    class ParameterReader


The methods look good to me. I'd like to get thoughts from @PhilipFackler and @superwhiskers on where these should live. I think it should be in GridKit/Model/.

superwhiskers

some quick comments. overall seems like a good change to make

superwhiskers · 2026-09-10T18:53:26Z

+     * @tparam ModelDataT A model data container exposing `RealT`, `IdxT`,
+     *         the `Parameters` enumeration, and the `parameters` map.
+     */
+    template <typename ModelDataT>


note: if/when #570 is merged, should probably require the ModelData concept present in GridKit/Model/PhasorDynamics/ModelData.hpp

you should be able to do this now

superwhiskers · 2026-09-10T18:54:38Z

+      /// Log one error against this model and count it.
+      std::ostream& fail()
+      {
+        ++error_count_;


i still think we should move away from counting errors and toward something more robust

lukelowry · 2026-09-15T22:15:09Z

I have not addressed every review comment but I have made larger cleanup and improvement, I think it's in the right direction.

lukelowry force-pushed the lukel/lambda-dev branch from d37b5c3 to 3b7ed08 Compare September 3, 2026 16:01

nkoukpaizan requested review from nkoukpaizan and superwhiskers September 3, 2026 20:16

nkoukpaizan assigned lukelowry Sep 3, 2026

nkoukpaizan added the enhancement New feature or request label Sep 3, 2026

lukelowry force-pushed the lukel/lambda-dev branch 3 times, most recently from 8523e16 to 1c5cb77 Compare September 9, 2026 20:19

lukelowry marked this pull request as ready for review September 10, 2026 00:11

nkoukpaizan reviewed Sep 10, 2026

View reviewed changes

superwhiskers reviewed Sep 10, 2026

View reviewed changes

lukelowry force-pushed the lukel/lambda-dev branch 3 times, most recently from 10d8a1a to 11adb87 Compare September 15, 2026 20:36

pelesh added this to the Release 0.3 milestone Sep 17, 2026

lukelowry and others added 5 commits September 18, 2026 20:54

initial lambda extraction

11e1902

older model migration and unify with new helper

a7cd7ba

Apply pre-commit fixes

3671628

Apply pre-commit fixes

cac9423

improved config checks and parameter reader

8a14a48

lukelowry force-pushed the lukel/lambda-dev branch from 11adb87 to 8a14a48 Compare September 19, 2026 02:20

Apply pre-commit fixes

863ca79

Navigation Menu

Initial lambda extraction - #566

Initial lambda extraction#566
lukelowry wants to merge 6 commits into
developfrom
lukel/lambda-dev

lukelowry commented Sep 2, 2026

Uh oh!

lukelowry commented Sep 10, 2026

Uh oh!

nkoukpaizan left a comment

Uh oh!

nkoukpaizan Sep 8, 2026

Uh oh!

nkoukpaizan Sep 8, 2026

Uh oh!

nkoukpaizan Sep 8, 2026

Uh oh!

nkoukpaizan Sep 8, 2026

Uh oh!

nkoukpaizan Sep 10, 2026

Uh oh!

superwhiskers left a comment

Uh oh!

superwhiskers Sep 10, 2026

Uh oh!

superwhiskers Sep 11, 2026

Uh oh!

superwhiskers Sep 10, 2026

Uh oh!

lukelowry commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lukelowry commented Sep 2, 2026

Description

Proposed changes

Checklist

Further comments

Uh oh!

lukelowry commented Sep 10, 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!

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

lukelowry commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants