Skip to content

Introduce CliArgs (new design) and use to further generalize ThreeBusBasicJson example#341

Open
PhilipFackler wants to merge 9 commits intodevelopfrom
PhilipFackler/threebusbasic-cliargs
Open

Introduce CliArgs (new design) and use to further generalize ThreeBusBasicJson example#341
PhilipFackler wants to merge 9 commits intodevelopfrom
PhilipFackler/threebusbasic-cliargs

Conversation

@PhilipFackler
Copy link
Collaborator

@PhilipFackler PhilipFackler commented Feb 23, 2026

Description

This is another step toward a generalized application to replace examples.

Proposed changes

Use a new class CliArgs to easily define and parse command line options. The usage design is based on ArgParse.jl.

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.
  • 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.

@PhilipFackler PhilipFackler added enhancement New feature or request examples labels Feb 23, 2026
@pelesh pelesh self-requested a review February 23, 2026 22:18
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@PhilipFackler PhilipFackler force-pushed the PhilipFackler/threebusbasic-cliargs branch 2 times, most recently from 7e50d41 to 326c561 Compare February 25, 2026 17:02
@PhilipFackler PhilipFackler force-pushed the PhilipFackler/threebusbasic-cliargs branch from e24bce8 to bbeef14 Compare February 27, 2026 17:49
@PhilipFackler PhilipFackler marked this pull request as ready for review February 27, 2026 18:03
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) << "";

@PhilipFackler PhilipFackler force-pushed the PhilipFackler/threebusbasic-cliargs branch from 9bc3ce1 to 22cb2e2 Compare March 2, 2026 15:30
@PhilipFackler PhilipFackler requested a review from pelesh March 2, 2026 15:31
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pelesh
Copy link
Collaborator

pelesh commented Mar 4, 2026

The updated code does not build on my machine:

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:
In file included from /Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/ArgVector.hpp:12:
/Users/55y/src/gridkit/GridKit/GridKit/Utilities/CliArgs/ArgValue.hpp:38:12: error: invalid operands to binary expression ('std::stringstream' (aka 'basic_stringstream<char>') and 'GridKit::Utilities::ArgValue')
   38 |         ss << val;
      |         ~~ ^  ~~~
/opt/local/libexec/llvm-18/bin/../include/c++/v1/__memory/construct_at.h:41:46: note: in instantiation of function template specialization 'GridKit::Utilities::ArgValue::ArgValue<GridKit::Utilities::ArgValue &>' requested here
   41 |   return ::new (std::__voidify(*__location)) _Tp(std::forward<_Args>(__args)...);
      |                                              ^
/opt/local/libexec/llvm-18/bin/../include/c++/v1/ostream:698:55: note: candidate function template not viable: no known conversion from 'GridKit::Utilities::ArgValue' to 'char' for 2nd argument
  698 | _LIBCPP_HIDE_FROM_ABI basic_ostream<_CharT, _Traits>& operator<<(basic_ostream<_CharT, _Traits>& __os, char __cn) {
      |                                                       ^                                                ~~~~~~~~~
/opt/local/libexec/llvm-18/bin/../include/c++/v1/ostream:725:53: note: candidate function template not viable: no known conversion from 'GridKit::Utilities::ArgValue' to 'char' for 2nd argument
  725 | _LIBCPP_HIDE_FROM_ABI basic_ostream<char, _Traits>& operator<<(basic_ostream<char, _Traits>& __os, char __c) {
      |                                                     ^                                              ~~~~~~~~
/opt/local/libexec/llvm-18/bin/../include/c++/v1/ostream:730:53: note: candidate function template not viable: no known conversion from 'GridKit::Utilities::ArgValue' to 'signed char' for

The output is truncated.

@PhilipFackler PhilipFackler force-pushed the PhilipFackler/threebusbasic-cliargs branch from 9e9e4ad to 748c1fc Compare March 6, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request examples

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants