Update to use ADTypes for specifying AD backend #338
Conversation
|
@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 |
|
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. |
|
@jClugstor can you accept those changes? That should fix the tests here and make it mergable. |
Co-authored-by: Christopher Rackauckas <[email protected]>
|
Looks like someone needs to approve the test workflows |
|
@jClugstor do you have any update on this PR? If there is any way I can help drive this forward let me know. |
|
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. |
|
Solve the other ones first |
|
The OOP tests were just a missing The AD tests have something to do with |
|
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). |
|
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. |
|
Thanks a lot for updating the test and making them run! What are the next steps then to get this PR over the finish line? |
|
It's missing a test dependency and import for SciMLStructures. |
|
Yes, I didn't see that there was a separate test project. The failures here aren't errors but test failures, it looks like ForwardDiff and FiniteDiff are giving different answers. |
|
That just looks like a tolerance thing. Lower the tolerance a bit on the solvers? |
|
Yes, lowering the tolerances made the tests pass locally. |
|
Yeah that's just a thing where adaptivity + autodiff means that you need sufficiently low tolerances for derivative convergence. That's not surprising given the default tolerances were a bit higher than the test tolerances. |
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (87.50%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
- Coverage 91.46% 91.35% -0.11%
==========================================
Files 44 44
Lines 2214 2221 +7
==========================================
+ Hits 2025 2029 +4
- Misses 189 192 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This looks good to merge now |
|
Thanks a lot for helping out with this! |
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.