Introduce CliArgs (new design) and use to further generalize ThreeBusBasicJson example#341
Open
PhilipFackler wants to merge 9 commits intodevelopfrom
Open
Introduce CliArgs (new design) and use to further generalize ThreeBusBasicJson example#341PhilipFackler wants to merge 9 commits intodevelopfrom
CliArgs (new design) and use to further generalize ThreeBusBasicJson example#341PhilipFackler wants to merge 9 commits intodevelopfrom
Conversation
pelesh
reviewed
Feb 24, 2026
Collaborator
pelesh
left a comment
There was a problem hiding this comment.
This is very welcome and long overdue update. Thanks for taking initiative on this. A few comments:
- Please document new functions with more detailed comments in the code.
- Consider separate source/header files for different classes. Some of the classes are only a few rows but it might be a good idea to stick to "one class per file" rule.
- I made some style suggestions that clang-format will not fix.
7e50d41 to
326c561
Compare
e24bce8 to
bbeef14
Compare
pelesh
requested changes
Feb 27, 2026
Collaborator
pelesh
left a comment
There was a problem hiding this comment.
Overall looks good, thank you! There is a couple of minor things that need to be fixed before we merge.
Test Fails
I am not sure why this fails because errors seem reasonable.
24: Test command: /Users/55y/src/gridkit/build-clang/examples/PhasorDynamics/Tiny/ThreeBus/Basic/ThreeBusBasicJson "--case" "ThreeBusBasic.json" "--compare" "mon.csv" "ThreeBusBasic.ref.csv"
24: Working Directory: /Users/55y/src/gridkit/build-clang/examples/PhasorDynamics/Tiny/ThreeBus/Basic
24: Test timeout computed to be: 1500
24: Example: ThreeBusBasicJson
24: Input file: ThreeBusBasic.json
24: Error Set:
24: Bus_1_Vm:
24: max : 5.700000e-08 (at time 4.167e-03)
24: L2-norm : 2.792418e-06
24: Bus_2_Vm:
24: max : 2.030698e-05 (at time 1.154e+00)
24: L2-norm : 1.037392e-04
24: Bus_3_Vm:
24: max : 4.015434e-05 (at time 1.154e+00)
24: L2-norm : 1.557142e-04
24: Genrou_genrou_2_1_speed:
24: max : 3.894410e-06 (at time 1.958e+00)
24: L2-norm : 4.565616e-05
24: Genrou_genrou_3_1_speed:
24: max : 7.913549e-06 (at time 1.583e+00)
24: L2-norm : 7.161058e-05
24: Total:
24: max : 4.015434e-05 (at time 1.154e+00)
24: L2-norm : 1.761691e-04
24: --- FAIL: Test main
24:
24:
24: Complete in 0.013721 seconds
1/1 Test #24: ThreeBusBasicJson ................***Failed 0.08 sec
Compiler warnings
Please fix remaining compiler warnings:
[ 32%] Linking CXX shared library libgridkit_power_elec_tranline.dylib
In file included from /Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/CliArgs.cpp:10:
In file included from /Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/CliArgs.hpp:15:
/Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/ArgVector.hpp:126:14: warning: class 'CliArgsImpl' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
126 | friend class CliArgsImpl;
| ^
/Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/ArgVector.hpp:20:12: note: previous use is here
20 | struct CliArgsImpl;
| ^
[ 32%] Linking CXX shared library libgridkit_solvers_steady.dylib
/Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/CliArgs.cpp:409:46: warning: implicit conversion changes signedness: 'unsigned int' to 'int' [-Wsign-conversion]
409 | os << std::setfill(' ') << std::setw(width) << "";
9bc3ce1 to
22cb2e2
Compare
pelesh
requested changes
Mar 2, 2026
Collaborator
pelesh
left a comment
There was a problem hiding this comment.
Unfortunately, the modified test still fails:
24: Test command: /Users/55y/src/gridkit/build-clang/examples/PhasorDynamics/Tiny/ThreeBus/Basic/ThreeBusBasicJson "--case" "ThreeBusBasic.json" "--compare" "mon.csv" "ThreeBusBasic.ref.csv"
24: Working Directory: /Users/55y/src/gridkit/build-clang/examples/PhasorDynamics/Tiny/ThreeBus/Basic
24: Test timeout computed to be: 1500
24: Example: ThreeBusBasicJson
24: Input file: ThreeBusBasic.json
24: Error Set:
24: Bus_1_Vm:
24: max : 5.700000e-08 (at time 4.167e-03)
24: L2-norm : 2.792418e-06
24: Bus_2_Vm:
24: max : 2.030698e-05 (at time 1.154e+00)
24: L2-norm : 1.037392e-04
24: Bus_3_Vm:
24: max : 4.015434e-05 (at time 1.154e+00)
24: L2-norm : 1.557142e-04
24: Genrou_genrou_2_1_speed:
24: max : 3.894410e-06 (at time 1.958e+00)
24: L2-norm : 4.565616e-05
24: Genrou_genrou_3_1_speed:
24: max : 7.913549e-06 (at time 1.583e+00)
24: L2-norm : 7.161058e-05
24: Total:
24: max : 4.015434e-05 (at time 1.154e+00)
24: L2-norm : 1.761691e-04
24: --- FAIL: Test main
24:
24:
24: Complete in 0.0072 seconds
1/1 Test #24: ThreeBusBasicJson ................***Failed 0.04 sec
0% tests passed, 1 tests failed out of 1
Total Test time (real) = 0.06 sec
The following tests FAILED:
24 - ThreeBusBasicJson (Failed)
Errors while running CTest
The max error looks good, though, I am not sure what triggers the failure.
Collaborator
|
The updated code does not build on my machine: The output is truncated. |
9e9e4ad to
748c1fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is another step toward a generalized application to replace examples.
Proposed changes
Use a new class
CliArgsto easily define and parse command line options. The usage design is based on ArgParse.jl.Checklist
-Wall -Wpedantic -Wconversion -Wextra.