Add more idaklu solver options

[MarcBerliner]

Description

Adds more user-configurable options to the IDA solver following the list in the IDA guide.

Some code cleanup to enforce early returns.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.45%. Comparing base (ab7348f) to head (d55a403).
Report is 20 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4249      +/-   ##
===========================================
- Coverage    99.55%   99.45%   -0.11%     
===========================================
  Files          288      288              
  Lines        21897    22088     +191     
===========================================
+ Hits         21800    21968     +168     
- Misses          97      120      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valentinsulzer]

Looks good to me, though I'm not familiar enough with C++ to properly review those files

[kratman]

@valentinsulzer I will review the C++ once the tests are passing

[kratman]

@MarcBerliner If you want the codecov failure to go away, rebasing should make the PR start using the correct commit. Since the review has not started yet, it is still fine to force push

[MarcBerliner]

@kratman It looks like the final real failure is with the static code analysis we discussed with the Options struct. Do you have a recommendation on how to proceed?

[kratman]

I will look at that with the code review

[kratman]

Partial review

[kratman]

@MarcBerliner The static analysis is correct. You are not using those variables. It is related to the scope issue that I mentioned in our call earlier:

CasadiSolverOpenMP::CasadiSolverOpenMP(
  np_array atol_np,
  double rel_tol,
  np_array rhs_alg_id,
  int number_of_parameters,
  int number_of_events,
  int jac_times_cjmass_nnz,
  int jac_bandwidth_lower,
  int jac_bandwidth_upper,
  std::unique_ptr<CasadiFunctions> functions_arg,
  const Options &options
) :
  atol_np(atol_np),
  rhs_alg_id(rhs_alg_id),
  number_of_states(atol_np.request().size),
  number_of_parameters(number_of_parameters),
  number_of_events(number_of_events),
  jac_times_cjmass_nnz(jac_times_cjmass_nnz),
  jac_bandwidth_lower(jac_bandwidth_lower),
  jac_bandwidth_upper(jac_bandwidth_upper),
  functions(std::move(functions_arg)),
  options(options)

At the bottom the member variable options has the same name as the input reference const Options &options

Then in your code you are using the following:

IDASetMaxOrd(ida_mem, options.max_order_bdf);

This is ambiguous due to the name options being in multiple scopes. This would be corrected by doing:

IDASetMaxOrd(ida_mem, this->options.max_order_bdf);

In C++ the this-> is effectively self. in python. When there is not scope ambiguity, then the this-> can be omitted. So what you wrote is basically the following:

self.options = options
IDASetMaxOrd(ida_mem, options.max_order_bdf)

instead of

self.options = options
IDASetMaxOrd(ida_mem, self.options.max_order_bdf)

So you never use the variables you added to the struct, you simply take those settings from the input variable.

If those variables are not needed anywhere else then you can look into inheriting from Options to make a new IKAOptions or something along those lines.

[kratman]

As a followup for making this easier, this problem would also be avoided by putting all the settings code into a method:

void CasadiSolverOpenMP::SetOptionsForIDA()
{
  // Maximum order of the linear multistep method
  IDASetMaxOrd(ida_mem, options.max_order_bdf);

  // Maximum number of steps to be taken by the solver in its attempt to reach
  // the next output time
  IDASetMaxNumSteps(ida_mem, options.max_num_steps);

  // Initial step size
  IDASetInitStep(ida_mem, RCONST(options.dt_init));

  // Maximum absolute step size
  IDASetMaxStep(ida_mem, RCONST(options.dt_max));

  // Maximum number of error test failures in attempting one step
  IDASetMaxErrTestFails(ida_mem, options.max_error_test_failures);

  // Maximum number of nonlinear solver iterations at one step
  IDASetMaxNonlinIters(ida_mem, options.max_nonlinear_iterations);

  // Maximum number of nonlinear solver convergence failures at one step
  IDASetMaxConvFails(ida_mem, options.max_convergence_failures);

  // Safety factor in the nonlinear convergence test
  IDASetNonlinConvCoef(ida_mem, RCONST(options.nonlinear_convergence_coefficient));

  // Suppress algebraic variables from error test
  IDASetSuppressAlg(ida_mem, options.suppress_algebraic_error);

  // Positive constant in the Newton iteration convergence test within the
  // initial condition calculation
  IDASetNonlinConvCoefIC(ida_mem, RCONST(options.nonlinear_convergence_coefficient_ic));

  // Maximum number of steps allowed when icopt=IDA_YA_YDP_INIT in IDACalcIC
  IDASetMaxNumStepsIC(ida_mem, options.max_num_steps_ic);

  // Maximum number of the approximate Jacobian or preconditioner evaluations
  // allowed when the Newton iteration appears to be slowly converging
  IDASetMaxNumJacsIC(ida_mem, options.max_number_jacobians_ic);

  // Maximum number of Newton iterations allowed in any one attempt to solve
  // the initial conditions calculation problem
  IDASetMaxNumItersIC(ida_mem, options.max_number_iterations_ic);

  // Maximum number of linesearch backtracks allowed in any Newton iteration
  //, when solving the initial conditions calculation problem
  IDASetMaxBacksIC(ida_mem, options.max_linesearch_backtracks_ic);

  // Turn off linesearch
  IDASetLineSearchOffIC(ida_mem, options.linesearch_off_ic);
}

This method takes no arguments, so all the data must be taken from the options. However, if those options are never used again, then maybe they do not need to be stored.

It is also not just your code that has this issue. It looks like right below your code the same mistake exists:

  // set events
  IDARootInit(ida_mem, number_of_events, events_casadi);
  void *user_data = functions.get();
  IDASetUserData(ida_mem, user_data);

  // specify preconditioner type
  precon_type = SUN_PREC_NONE;
  if (options.preconditioner != "none") {
    precon_type = SUN_PREC_LEFT;
  }

Here options and number_of_events are also taken from the arguments, not the member data of the class.

[MarcBerliner]

Closing due to merging #4282