Command-line control over scalar-valued set routines #662
Conversation
drreynolds
requested review from
balos1,
gardner48 and
cswoodward
as code owners
| SUNDIALS_EXPORT int ARKodeSetFromCommandLine(void* arkode_mem, int argc, | ||
| char* argv[]); |
There was a problem hiding this comment.
| SUNDIALS_EXPORT int ARKodeSetFromCommandLine(void* arkode_mem, int argc, | |
| char* argv[]); | |
| SUNDIALS_EXPORT int ARKodeSetFromCommandLine(void* arkode_mem, int argc, | |
| const char* argv[]); |
| int num_int_keys = 15; | ||
| const char* int_keys[] = {"arkode.order", | ||
| "arkode.interpolant_degree", | ||
| "arkode.linear", | ||
| "arkode.autonomous", | ||
| "arkode.deduce_implicit_rhs", | ||
| "arkode.lsetup_frequency", | ||
| "arkode.predictor_method", | ||
| "arkode.max_nonlin_iters", | ||
| "arkode.max_hnil_warns", | ||
| "arkode.interpolate_stop_time", | ||
| "arkode.max_num_constr_fails", | ||
| "arkode.adaptivity_adjustment", | ||
| "arkode.small_num_efails", | ||
| "arkode.max_err_test_fails", | ||
| "arkode.max_conv_fails"}; | ||
| arkIntSetFn int_set[] = {ARKodeSetOrder, | ||
| ARKodeSetInterpolantDegree, | ||
| ARKodeSetLinear, | ||
| ARKodeSetAutonomous, | ||
| ARKodeSetDeduceImplicitRhs, | ||
| ARKodeSetLSetupFrequency, | ||
| ARKodeSetPredictorMethod, | ||
| ARKodeSetMaxNonlinIters, | ||
| ARKodeSetMaxHnilWarns, | ||
| ARKodeSetInterpolateStopTime, | ||
| ARKodeSetMaxNumConstrFails, | ||
| ARKodeSetAdaptivityAdjustment, | ||
| ARKodeSetSmallNumEFails, | ||
| ARKodeSetMaxErrTestFails, | ||
| ARKodeSetMaxConvFails}; |
There was a problem hiding this comment.
| int num_int_keys = 15; | |
| const char* int_keys[] = {"arkode.order", | |
| "arkode.interpolant_degree", | |
| "arkode.linear", | |
| "arkode.autonomous", | |
| "arkode.deduce_implicit_rhs", | |
| "arkode.lsetup_frequency", | |
| "arkode.predictor_method", | |
| "arkode.max_nonlin_iters", | |
| "arkode.max_hnil_warns", | |
| "arkode.interpolate_stop_time", | |
| "arkode.max_num_constr_fails", | |
| "arkode.adaptivity_adjustment", | |
| "arkode.small_num_efails", | |
| "arkode.max_err_test_fails", | |
| "arkode.max_conv_fails"}; | |
| arkIntSetFn int_set[] = {ARKodeSetOrder, | |
| ARKodeSetInterpolantDegree, | |
| ARKodeSetLinear, | |
| ARKodeSetAutonomous, | |
| ARKodeSetDeduceImplicitRhs, | |
| ARKodeSetLSetupFrequency, | |
| ARKodeSetPredictorMethod, | |
| ARKodeSetMaxNonlinIters, | |
| ARKodeSetMaxHnilWarns, | |
| ARKodeSetInterpolateStopTime, | |
| ARKodeSetMaxNumConstrFails, | |
| ARKodeSetAdaptivityAdjustment, | |
| ARKodeSetSmallNumEFails, | |
| ARKodeSetMaxErrTestFails, | |
| ARKodeSetMaxConvFails}; | |
| static const char* int_keys[] = {"arkode.order", | |
| "arkode.interpolant_degree", | |
| "arkode.linear", | |
| "arkode.autonomous", | |
| "arkode.deduce_implicit_rhs", | |
| "arkode.lsetup_frequency", | |
| "arkode.predictor_method", | |
| "arkode.max_nonlin_iters", | |
| "arkode.max_hnil_warns", | |
| "arkode.interpolate_stop_time", | |
| "arkode.max_num_constr_fails", | |
| "arkode.adaptivity_adjustment", | |
| "arkode.small_num_efails", | |
| "arkode.max_err_test_fails", | |
| "arkode.max_conv_fails"}; | |
| static arkIntSetFn int_set[] = {ARKodeSetOrder, | |
| ARKodeSetInterpolantDegree, | |
| ARKodeSetLinear, | |
| ARKodeSetAutonomous, | |
| ARKodeSetDeduceImplicitRhs, | |
| ARKodeSetLSetupFrequency, | |
| ARKodeSetPredictorMethod, | |
| ARKodeSetMaxNonlinIters, | |
| ARKodeSetMaxHnilWarns, | |
| ARKodeSetInterpolateStopTime, | |
| ARKodeSetMaxNumConstrFails, | |
| ARKodeSetAdaptivityAdjustment, | |
| ARKodeSetSmallNumEFails, | |
| ARKodeSetMaxErrTestFails, | |
| ARKodeSetMaxConvFails}; | |
| static const int num_int_keys = sizeof(int_keys) / sizeof(*int_keys); |
and similar changes for code below...
To avoid issues with maintaining consistent lengths of these arrays, a cleaner solution might be to have a single array of struct { char* name; arkIntSetFn fun}.
Alternatively, this seems like an ideal place to use SUNDIALS' hashmap.
| *arg_used = SUNFALSE; | ||
| if (strcmp(argv[*i], argtest) == 0) | ||
| { | ||
| int iarg = atoi(argv[++(*i)]); |
There was a problem hiding this comment.
This usage of ++ is confusing. Can it be done on a separate line?
| sunbooleantype *arg_used) | ||
| { | ||
| *arg_used = SUNFALSE; | ||
| if (strcmp(argv[*i], argtest) == 0) |
There was a problem hiding this comment.
So booleans are specified with just the flag? I think it would be preferable to have flag-value pair command line syntax for boolean options. Otherwise, a user will need to know the default and whether to override it.
| sunbooleantype arg_used = SUNFALSE; | ||
|
|
||
| /* skip command-line arguments that do not begin with "arkode." */ | ||
| if (strncmp(argv[i], "arkode.", 7) != 0) { continue; } |
There was a problem hiding this comment.
Since we filter by the arkode. prefix here, I think it's redundant to include it in all the array entries above. We would just need to shift the string pointer below.
There was a problem hiding this comment.
Possibly out of scope for this initial PR, but in order to support full configuration over an MRI or operator splitting method where they may be several arkode instances, we may need to support user-specified prefixes. For example, arkode.order 2 partition1.order 2 partition2.linear 1
| sunbooleantype arg_used = SUNFALSE; | ||
|
|
||
| /* skip command-line arguments that do not begin with "arkode." */ | ||
| if (strncmp(argv[i], "arkode.", 7) != 0) { continue; } |
There was a problem hiding this comment.
Removes "magic constant"
| if (strncmp(argv[i], "arkode.", 7) != 0) { continue; } | |
| static const char *prefix = "arkode."; | |
| if (strncmp(argv[i], prefix, strlen(prefix)) != 0) { continue; } |
| if (strcmp(argv[i], "arkode.interpolant_type") == 0) | ||
| { | ||
| i++; | ||
| retval = 1; |
There was a problem hiding this comment.
| retval = 1; | |
| retval = ARK_ILL_INPUT; |
| arkProcessError(ark_mem, ARK_ILL_INPUT, __LINE__, __func__, __FILE__, | ||
| "error setting command-line argument: %s %s", | ||
| argv[i-1], argv[i]); | ||
| return ARK_ILL_INPUT; |
There was a problem hiding this comment.
It seems this function always returns ARK_ILL_INPUT on a failure. If the input was ok, but a function like ARKodeSetInterpolantType failed for a different reason, e.g., some internal interpolation memory was corrupted, it would be more informative to pass along that error.
I'm opening this up as a draft PR to get feedback on the approach before I update the other packages (CVODE, CVODES, IDA, IDAS, KINSOL), and the various class implementations (
SUNLinearSolver,SUNAdaptController,SUNNonlinearSolver, etc).We can discuss this during an upcoming group meeting, if desired. @balos1 and I have scaled back the previous plans to account for the reduced manpower allocated to the task, so we're only focusing on command-line control for now (YAML or other file-based inputs have been deferred).