Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move DAE init alg error message further down the dependency stack #2554

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Ickaser
Copy link

@Ickaser Ickaser commented Dec 9, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Still working on #2513 ; this pull request attempts to address that issue in a non-breaking way, whereas #2514 solves it more elegantly but with a breaking change to the default initialization algorithm. (That will probably need to be rebased on this pull request once this works as desired.)

Attempted approach

BrownFullBasicInit and ShampineCollocationInit both require OrdinaryDiffEqNonlinearSolve. The original implementation had an error message when the nonlinear solve is attempted if ODENLSolve is not loaded, but that nonlinear solve happens in a method of _initialize_dae! which is defined only in ODENLSolve, so the error message needs to be placed in ODECore. This attempt currently checks in the DefaultInit method for _initialize_dae! whether ODENLSolve is loaded, and if it is, proceeds to use default algorithms as done previously. If not loaded, then an error is thrown.

I don't feel like enough of an expert to say authoritatively what the best way is to check if ODENLSolve is loaded. From what I could find of discussions on Discourse, etc., it seems like the recommended way might be to do

if !isdefined(Main, :OrdinaryDiffEqNonlinearSolve)

so that is what I have done here.

Two other approaches I turned up (but that seem brittle or discouraged) might be something along the lines of

if !hasmethod(_initialize_dae!, Tuple{Any, ODEProblem, BrownFullBasicInit, typeof(isinplace))

or checking if ODENLSolve is in Base.loaded_modules.

@ChrisRackauckas
Copy link
Member

Looks like this is a bit too trigger happy in the tests.

@Ickaser
Copy link
Author

Ickaser commented Dec 23, 2024

The reason this was too trigger-happy:

if !isdefined(Main, :OrdinaryDiffEqNonlinearSolve)

fails if the user (or tester) loads ODENLSolve via using OrdinaryDiffEq or using DifferentialEquations. We could try to make a list of the packages that all eventually load ODENLSolve and check if any of those are loaded, but then if a user makes their own package and @reexports ODENLSolve (which is something I have done myself) then this breaks that. That might look like the following:

const nlsolve_umbrella_pkgs = [:OrdinaryDiffEqNonlinearSolve, :OrdinaryDiffEq, :DifferentialEquations]
if all([!isdefined(Main, pkg) for pkg in nlsolve_umbrella_pkgs])

As I said, though, that breaks if someone tries to reexport in their own package. So in the end I think directly checking if the methods that get dispatched from the default algorithm exist is the only non-breaking way to introduce this informative error. That ends up looking like the following, where we basically check if the method exists, and call it if it does:

    elseif !applicable(_initialize_dae!, integrator, prob,
        BrownFullBasicInit(integrator.opts.abstol), x)
        error("`OrdinaryDiffEqNonlinearSolve` is not loaded, which is required for the default initialization algorithm (`BrownFullBasicInit` or `ShampineCollocationInit`). To solve this problem, either do `using OrdinaryDiffEqNonlinearSolve` or pass `initializealg = CheckInit()` to the `solve` function. This second option requires consistent `u0`.")
    else
        _initialize_dae!(integrator, prob,
            BrownFullBasicInit(integrator.opts.abstol), x)

Unlike my first approach, this seems to actually allow tests to pass, at least as far as the tests have gotten on my machine while I write.

@Ickaser
Copy link
Author

Ickaser commented Dec 23, 2024

If this new approach looks alright and works on existing tests, then I suppose a couple tests should get added somewhere to make sure that this error works as intended? If so, those tests will need to be removed by the v7 commit that addresses this issue more elegantly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants