Skip to content

Make get_differential_vars type stable #2698

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 1 commit into
base: master
Choose a base branch
from

Conversation

Ickaser
Copy link
Contributor

@Ickaser Ickaser commented May 5, 2025

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

Solves #2594 , based on #2594 (comment).

Question on testing

It seems like a good idea to add a test that would have caught #2594; my attempt would be to add @inferred in a couple places. Would

@testset "Inplace: $(isinplace(_prob)), DAEProblem: $(_prob isa DAEProblem), BrownBasic: $(initalg isa BrownFullBasicInit), Autodiff: $autodiff" for _prob in [
prob_mm, prob_mm_oop],
initalg in [BrownFullBasicInit(), ShampineCollocationInit()], autodiff in [true, false]
alg = Rodas5P(; autodiff)
function f(p)
sol = solve(remake(_prob, p = p), alg, abstol = 1e-14,
reltol = 1e-14, initializealg = initalg)
sum(sol)
end
@test ForwardDiff.gradient(f, [0.04, 3e7, 1e4])[0, 0, 0] atol=1e-8
end

or
p = [0.04, 3e7, 1e4]
u₀ = [1.0, 0, 0]
du₀ = [-0.04, 0.04, 0.0]
tspan = (0.0, 100000.0)
differential_vars = [true, true, false]
prob = DAEProblem(f, du₀, u₀, tspan, p, differential_vars = differential_vars)
prob_oop = DAEProblem{false}(f, du₀, u₀, tspan, p, differential_vars = differential_vars)
sol1 = solve(prob, DFBDF(), dt = 1e-5, abstol = 1e-8, reltol = 1e-8)
sol2 = solve(prob_oop, DFBDF(), dt = 1e-5, abstol = 1e-8, reltol = 1e-8)
# These tests flex differentiation of the solver and through the initialization
# To only test the solver part and isolate potential issues, set the initialization to consistent
@testset "Inplace: $(isinplace(_prob)), DAEProblem: $(_prob isa DAEProblem), BrownBasic: $(initalg isa BrownFullBasicInit), Autodiff: $autodiff" for _prob in [
prob, prob_oop],
initalg in [BrownFullBasicInit(), ShampineCollocationInit()], autodiff in [true, false]
alg = DFBDF(; autodiff)
function f(p)
sol = solve(remake(_prob, p = p), alg, abstol = 1e-14,
reltol = 1e-14, initializealg = initalg)
sum(sol)
end
@test ForwardDiff.gradient(f, [0.04, 3e7, 1e4])[0, 0, 0] atol=1e-8
end

be a good place for these? Should this get tested with all the implicit solvers where the mass-matrix DAE formulation is allowed?

@ChrisRackauckas
Copy link
Member

be a good place for these?

Yeah, the more the marrier.

https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/test/interface/mass_matrix_tests.jl is probably a good test to slam some around.

@Ickaser
Copy link
Contributor Author

Ickaser commented May 5, 2025

Turns out that lots of these are still not inferrable--even with this PR, going from Rosenbrock DAE AD tests, the move from

sol = solve(prob_mm, Rodas5P(), reltol = 1e-8, abstol = 1e-8)

to

using ADTypes: AutoForwardDiff
sol = @inferred solve(prob_mm, Rodas5P(autodiff=AutoForwardDiff(chunksize=3)), reltol = 1e-8, abstol = 1e-8)

already runs into an issue, since the type of solve is still inferred to be Any. I think there is something else going on here (or I don't know how to use @inferred properly, which is possible), but I can't tell what makes this different from the MWE I've been testing against.

That said, even though this PR apparently doesn't do enough for the existing test suite, it is already helping with the MWE and therefore probably my code base.

@ChrisRackauckas
Copy link
Member

It's fine to sprinkle some @test_brokens around to get things in, and document them.

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