Skip to content

Migrate solve dispatches from DiffEqBase to NonlinearSolveBase #669

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jClugstor
Copy link
Member

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

@@ -12,7 +12,7 @@ using LineSearch: BackTracking
using StaticArraysCore: SArray

using CommonSolve: CommonSolve
using DiffEqBase: DiffEqBase # Needed for `init` / `solve` dispatches
#using DiffEqBase: DiffEqBase # Needed for `init` / `solve` dispatches
Copy link
Member

Choose a reason for hiding this comment

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

not a comment, just delete.

@@ -1,3 +1,449 @@
const allowedkeywords = (:dense,
Copy link
Member

Choose a reason for hiding this comment

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

The utilities at the top here are reused by DiffEqBase, NonlinearSolveBase, OptimizationBase (in the near future), etc. Let's move these to SciMLBase and share them across the different repos.

Copy link
Member

Choose a reason for hiding this comment

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

The allowedkwargs handling, the special error types, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so too, but I figured it might be nice for each to have a separate list of acceptable keywords, so if you tried to pass e.g. dtmin in to a NonlinearSolve it would give a nice error. Also the messages right now are specific to DiffEqs, they point to the DiffEq docs. I made it so that if there's a mismatch kwarg for a nonlinear solve it points to the NonlinearSolve docs.

But yeah if these are better in SciMLBase I can just put them there.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also a lot of small utility functions that live in DiffEqBase right now, e.g.
get_concrete_problem, extract_alg, promote_u0, anyeltypedual etc. that get used in the solve functions. I'm thinking those should also be moved to SciMLBase?

Copy link
Member

Choose a reason for hiding this comment

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

The list of acceptable kwargs, yes that one should be problem specific, that would be a nice improvement.

But the others yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently the definitions for functions like promote_u0 and anyeltypedual etc. are in different extensions in DiffEqBase. I'm thinking I should create corresponding extensions in SciMLBase for those packages and move the functions to those. But I'll keep the parts of the extensions specific to DiffEqs in the extensions in DiffEqBase.

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