-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
[WIP] Update to Sundials 5 #235
Conversation
Hi Chris. I'm also working with @jd-lara in the update. We are still working on the wrapped api and will have to go the additional constants and types that were added on Sundials 5. For now we don't need help, but probably will need some help later in testing new features, like selecting non linear solvers or new parameters. |
[WIP] Update ERK and MRI
@ChrisRackauckas and @YingboMa The PR is ready for review. Tests are passing and ERK is enabled. MRI is still work in progress but I don't think that should be blocked since it wasn't part of Sundials 3. We will keep working on it ask the Sundials guys about it. |
struct PTFQMR <: SundialsLinearSolver end | ||
struct KLU <: SundialsLinearSolver end | ||
struct LapackBand <: SundialsLinearSolver end | ||
struct LapackDense <: SundialsLinearSolver end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to default to the Lapack versions. Also, let's make sure the docs get updated to add these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an action item for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action item for @YingboMa to be aware of.
To be clear, the plan will be to bring this into master, but no tag or release will be done until the common interface is fixed and worked. The tag will obviously be v4.0. Sundials split and renamed ARKODE -> ARKstep and ERKstep, and so we can do a deprecation along those lines too, but should be a proper deprecation so that users get auto-forwarded. For the v4.0 release, I am going to say that let's not worry about KLU for now, but that should be on our minds to get fixed up. Courtesy ping to @MartinOtter on this. |
Looks like the review only found a bunch of small things, so we can get that quickly cleaned up and merged. |
I will take care of the comments today. |
I committed updates to the common interface here https://github.com/SciML/Sundials.jl/commits/jd/sundials5 |
Comments were taken care of, issues were opened for what is left, so I will merge when tests pass and then Yingbo you should rebase and PR to master. |
Just to add to this, there is a new method for multi-rate ODE's (MRI) that are still trying to implement. There are some kinks with the API but should be a new integrator and structure. |
Cheers. Great work @jd-lara . I'll invite you to the Sundials team. |
Opening the PR to track it.
@jd-lara where is this at right now? Need help with anything?