Skip to content

Update to use ADTypes for specifying AD backend #338

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jClugstor
Copy link

Recently OrdinaryDiffEq has been upgraded to using ADTypes to specify the AD backend used in implicit solvers.
Additionally we'll be updating to use DifferentiationInterface, SciML/OrdinaryDiffEq.jl#2567.

This PR makes EK1 and EK1Diagonal use ADTypes to specify the AD backend, to make ProbNumDiffEq compatible with the upcoming versions of OrdinaryDiffEq.

@nathanaelbosch
Copy link
Owner

@jClugstor Thanks a lot for opening this PR! Do you need any help to get this PR forward and fix the failing tests? I don't have a good overview over the AD changes going on in OrdinaryDiffEq, but in case there are any blockers please let me know

@jClugstor
Copy link
Author

The tests should hopefully pass once the newest version of OrdinaryDiffEqDifferentiation comes out, which will hopefully be very soon. There are some changes in it that will break some parts of ProbNumDiffEq, so this PR should fix it.

@ChrisRackauckas
Copy link
Contributor

@jClugstor can you accept those changes? That should fix the tests here and make it mergable.

Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
@jClugstor
Copy link
Author

Looks like someone needs to approve the test workflows

@nathanaelbosch
Copy link
Owner

@jClugstor do you have any update on this PR? If there is any way I can help drive this forward let me know.

@jClugstor
Copy link
Author

So it looks like the failures are related to some of the ModelingToolkit indexing stuff, which I really don't know much about. I think I should be able to look in to it pretty soon though, hopefully sometime next week.

@ChrisRackauckas
Copy link
Contributor

Solve the other ones first

@jClugstor
Copy link
Author

The OOP tests were just a missing constructorof for ConstructionBase/Accessors.

The AD tests have something to do with remake on an ODEProblem coming from MTK.

@nathanaelbosch
Copy link
Owner

FYI I just changed a setting in the repo so that hopefully I won't have to approve workflows anymore (though I'll continue to check regularly to make sure that that's not a blocker).

@jClugstor
Copy link
Author

This should let the tests run without error at least, it just needed to use SciMLStructures to give the finite diff gradients something they can actually use.

The tests still fail though, there's a bit of a difference between the output of ForwardDiff and FiniteDifferences here. I also tested FiniteDiff, and there's still a difference that's greater than the tolerance.

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.

3 participants