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

Fix inconsistencies of type parameters of algorithms with AD settings #2617

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Mar 3, 2025

I'm aware of the work on #2567, but for a downstream application (Pumas, to be precise) I'd like a hotfix release with a bugfix for #2613: We see severe CI time regressions with the latest version of the SciML stack, and the most likely culprit is #2613 - and in particular the fact that it implies that fine-tuned chunksize heuristics are not applied anymore.

I inspected the OrdinaryDiffEq code and it seems that the change to AD types lead to an inconsistency of the type parameters of the AD-dependent algorithms: The chunksize type parameter is set based on chunk_size, the AD type parameter based on autodiff, and the finitediff type parameter based on diff_type. However, the default chunksize (0) and diff type (Val{:forward}) parameters are used even if an AD type with differing chunksize or diff type is specified. This PR fixes this inconsistency by instead using the chunksize and diff type of the AD type (unless non-default values are used for those, in which case the AD type is considered just as a proxy for the previous Val{true}/Val{false} scheme - this is already the design policy on the master branch).

Fixes #2613.

@oscardssmith
Copy link
Member

looks reasonable to me, but I wouldn't mind a double check from @jClugstor

Copy link
Member

@jClugstor jClugstor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Only thing I'm wondering is why change the default diff_type from central to forward in e.g. RosenbrockW6S4OS and a few other places?

@@ -160,10 +160,10 @@ struct RosenbrockW6S4OS{CS, AD, F, P, FDT, ST, CJ} <:
end
function RosenbrockW6S4OS(; chunk_size = Val{0}(), autodiff = AutoForwardDiff(),
standardtag = Val{true}(),
concrete_jac = nothing, diff_type = Val{:central},
concrete_jac = nothing, diff_type = Val{:forward}(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout the package a default of diff_type = Val{:forward}() is used and documented (similar to the default of chunk_size = 0). Hence I assumed this is an unintentional inconsistency. If it's desired, the _process... function has to be changed and algorithm-specific default values of chunk_size and diff_type have to be forwarded to it.

@devmotion
Copy link
Member Author

why change the default diff_type from central to forward in e.g. RosenbrockW6S4OS and a few other places?

The only change is RosenbrockW6S4OS. All other algorithms already use diff_type = Val{:forward}() as default value.

@devmotion
Copy link
Member Author

Anything to change or add in this PR? Is it fine to make RosenbrockW6S40S consistent with all other algorithms?

As far as I can tell the remaining test failures are not introduced by this PR. The GPU test failure is caused by a buggy test in DiffEqBase: SciML/DiffEqBase.jl#1122

@ChrisRackauckas ChrisRackauckas merged commit a0dbb46 into master Mar 4, 2025
135 of 145 checks passed
@ChrisRackauckas ChrisRackauckas deleted the dw/adtype branch March 4, 2025 13:19
@devmotion
Copy link
Member Author

Would it be possible to tag a new release of OrdinaryDiffEq? Or is master in an unstable state currently?

@ChrisRackauckas
Copy link
Member

All masters should be fine. Release all of the packages you updated.

@devmotion
Copy link
Member Author

Hmm I've never made releases in a repo with multiple subpackages... How does it even work? Usually I just comment on the git commit that bumps the Project.toml.

@ChrisRackauckas
Copy link
Member

That's why I just use the register issue.

@devmotion
Copy link
Member Author

What's the semver policy with these subpackages? In principle, we need

  1. a breaking release of OrdinaryDiffEqCore (otherwise it breaks the released versions of the other ADTypes-dependent subpackages touched by the PR)
  2. non-breaking releases of the other subpackages modified by the PR that switch them over to the breaking release of OrdinaryDiffEqCore

But to be able to use OrdinaryDiffEq (and not only these subpackages) it seems 1. also implies that all other subpackages + OrdinaryDiffEq require a new release.

Alternatively, we could add back the old _process_AD_choice methods in OrdinaryDiffEqCore and rename the fixed version to e.g. _process_autodiff_chunk_size_diff_type. Then the release process would be

  1. a non-breaking release of OrdinaryDiffEqCore
  2. non-breaking releases of the other subpackages modified by the PR that switch them over to the new non-breaking release of OrdinaryDiffEqCore (just updating the compat entry to disallow the older versions)

@ChrisRackauckas
Copy link
Member

It's a non-breaking release of OrdinaryDiffEqCore. We're basically developing with JuliaLang/julia#55516 without the feature existing.

@devmotion
Copy link
Member Author

But as soon as I make a non-breaking release of OrdinaryDiffEqCore, packages such as OrdinaryDiffEqRosenbrock, OrdinaryDiffEq, etc. will all be broken? Is that still what you suggest?

@ChrisRackauckas
Copy link
Member

Yeah and bump the minor, and bump the Julialang issue so we eventually get this fixed. There really isn't a better way unless you want to spend an hour on every release.

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.

AutoForwardDiff (chunksize setting?) breaks type inference of solve
4 participants