Skip to content
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

Switch to using ADTypes for AD choice #2508

Merged
merged 88 commits into from
Dec 21, 2024

Conversation

jClugstor
Copy link
Contributor

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

This will switch over to using ADTypes for the autodiff keyword in the construction of the solver types.

  • Changes the default autodiff keyword to be AutoForwardDiff() instead of true.
  • Adds a deprecation path so that using true or false for autodiff uses AutoForwardDiff() and AutoFiniteDiff() respectively.
  • Also ensures that if the chunk_size and diff_type kwargs are used with a Bool autodiff, they are respected.

@jClugstor
Copy link
Contributor Author

@oscardssmith It's able to precompile in this state, so at least anything in the precompile workloads will work. I'm pretty sure some tests won't pass though, and I still need to add some tests as well.

@jClugstor jClugstor force-pushed the ADTypesSwitch branch 6 times, most recently from 5aab8fc to 4ada725 Compare November 12, 2024 15:07
@jClugstor
Copy link
Contributor Author

@oscardssmith All CI tests are passing now. Some integration tests are broken.

@oscardssmith
Copy link
Contributor

The StochasticDiffEq error looks very odd. @gdalle any idea what would cause this?

 In-place methods: Test Failed at /home/runner/work/OrdinaryDiffEq.jl/OrdinaryDiffEq.jl/downstream/test/complex_tests.jl:42
  Expression: solve(prob, alg)
    Expected: ArgumentError
      Thrown: DivideError
      DivideError: integer division error
      Stacktrace:
        [1] div
          @ ./int.jl:295 [inlined]
        [2] div
          @ ./div.jl:310 [inlined]
        [3] div
          @ ./div.jl:365 [inlined]
        [4] cld
          @ ./div.jl:322 [inlined]
        [5] generate_chunked_partials(x::Matrix{ComplexF64}, colorvec::UnitRange{Int64}, cs::Val{0})
          @ SparseDiffTools ~/.julia/packages/SparseDiffTools/m4eae/src/differentiation/compute_jacobian_ad.jl:80
        [6] SparseDiffTools.ForwardColorJacCache(f::SciMLBase.UJacobianWrapper{true, SciMLBase.SDEFunction{true, SciMLBase.FullSpecialize, Main.var"##Complex Number Tests#233".var"#f#2", Main.var"##Complex Number Tests#233".var"#f#2", LinearAlgebra.UniformScaling{Bool}, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, typeof(SciMLBase.DEFAULT_OBSERVED), Nothing, Nothing, Nothing}, Float64, SciMLBase.NullParameters}, x::Matrix{ComplexF64}, _chunksize::Val{0}; dx::Nothing, tag::Type, colorvec::UnitRange{Int64}, sparsity::Nothing)
          @ SparseDiffTools ~/.julia/packages/SparseDiffTools/m4eae/src/differentiation/compute_jacobian_ad.jl:37
        [7] ForwardColorJacCache
          @ ~/.julia/packages/SparseDiffTools/m4eae/src/differentiation/compute_jacobian_ad.jl:22 [inlined]

@gdalle
Copy link

gdalle commented Nov 12, 2024

It appears you have a chunksize of 0 somewhere. Maybe an ill-advised choice in AutoForwardDiff(; chunksize=...)? The default is nothing and not 0.

@jClugstor
Copy link
Contributor Author

Yeah, my guess is that it has something to do with the chunksize,

@jClugstor
Copy link
Contributor Author

I think it's because the fallback _get_fwd_chunksize(AD) = Val(0). Since the Stochastic algs aren't using ADTypes yet, it falls back to that inside of get_chunksize.

@gdalle
Copy link

gdalle commented Nov 12, 2024

If anything, that fallback should be Val(1). Unless we're explicitly trying to throw an error by using zero.

@oscardssmith
Copy link
Contributor

What's happening here is that we were using Val(0) to mean automatic chunksize construction. Does DI have a good way to ask for that?

@gdalle
Copy link

gdalle commented Nov 12, 2024

That's what AutoForwardDiff(; chunksize=nothing) does. Do you need to manipulate chunk sizes for other backends? if so, why?

@jClugstor
Copy link
Contributor Author

Actually, I think I'm wrong, for a Stochastic algorithm, it should be getting the chunksize from one of these.

OrdinaryDiffEq.get_chunksize(alg::StochasticDiffEqNewtonAlgorithm{CS,AD,FDT,ST,CJ,Controller}) where {CS,AD,FDT,ST,CJ,Controller} = Val(CS)
OrdinaryDiffEq.get_chunksize(alg::StochasticDiffEqNewtonAdaptiveAlgorithm{CS,AD,FDT,ST,CJ,Controller}) where {CS,AD,FDT,ST,CJ,Controller} = Val(CS)
OrdinaryDiffEq.get_chunksize(alg::StochasticDiffEqJumpNewtonAdaptiveAlgorithm{CS,AD,FDT,ST,CJ,Controller}) where {CS,AD,FDT,ST,CJ,Controller} = Val(CS)
OrdinaryDiffEq.get_chunksize(alg::StochasticDiffEqJumpNewtonDiffusionAdaptiveAlgorithm{CS,AD,FDT,ST,CJ,Controller}) where {CS,AD,FDT,ST,CJ,Controller} = Val(CS)

@gdalle
Copy link

gdalle commented Nov 12, 2024

Note that depending on the semantics of chunk size in these libraries (which I don't know), setting nothing as the default may not work either. All I can comment on is ADTypes and DifferentiationInterface:

  • In ADTypes, Auto(Polyester)ForwardDiff is the only backend with a configurable chunk size, and it can be either nothing or an integer > 0
  • In DI, I leverage the choice of chunk size in AutoForwardDiff and set it to 1 for every other backend (except Enzyme which has a custom rule). Chunk size handling is internal right now because I'm not yet sure I have nailed the right API.

@gdalle
Copy link

gdalle commented Nov 12, 2024

To get a clearer idea of which tests are actually failing and which tests are just Codecov complaining about your uploads, you can temporarily set these settings to false:

fail_ci_if_error: true

@jClugstor
Copy link
Contributor Author

Oh thanks, that's helpful.

@jClugstor
Copy link
Contributor Author

@oscardssmith all CI tests pass, and all of the failing integration tests are failing on master as well.

@jClugstor jClugstor marked this pull request as ready for review November 13, 2024 14:31
@oscardssmith
Copy link
Contributor

looks good! Thanks so much

@gdalle
Copy link

gdalle commented Nov 15, 2024

This Codecov throttling is really frustrating

@ChrisRackauckas
Copy link
Member

Yeah and we need to delete it repo by repo for ones that don't use the high level CI description 😱 . At least there aren't too many repos left that need to do it.

@jClugstor jClugstor force-pushed the ADTypesSwitch branch 2 times, most recently from f05b76d to 0c22789 Compare December 11, 2024 15:42
@jClugstor
Copy link
Contributor Author

I believe all the tests that are failing here are also failing on master.

@jClugstor
Copy link
Contributor Author

@ChrisRackauckas @oscardssmith I think this is ready

@ChrisRackauckas ChrisRackauckas merged commit 3921078 into SciML:master Dec 21, 2024
30 of 49 checks passed
@ChrisRackauckas
Copy link
Member

Can you make sure to do the downstream in StochasticDiffEq and DelayDiffEq?

@jClugstor
Copy link
Contributor Author

Yes, will do.

@gdalle
Copy link

gdalle commented Dec 23, 2024

hot damn, this is a great christmas gift! thanks for all your hard work

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.

4 participants