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

Consistency in sensealg arguments #1088

Closed
mschytt opened this issue Aug 1, 2024 · 1 comment · Fixed by #1106
Closed

Consistency in sensealg arguments #1088

mschytt opened this issue Aug 1, 2024 · 1 comment · Fixed by #1106
Assignees

Comments

@mschytt
Copy link

mschytt commented Aug 1, 2024

Hi👋

After a small back-and-forth with Chris on slack, we found that the direct sensitivity analysis interfaces for ODEs are not consistent between the forward and adjoint methods. Specifically, I am considering the choice of sensitivity algorithm.

The documentation for ODEForwardSensitivityProblem gives the following signature with alg as a positional argument:

help?> ODEForwardSensitivityProblem

search: ODEForwardSensitivityProblem ODEForwardSensitivityFunction

  function ODEForwardSensitivityProblem(f::Union{Function,DiffEqBase.AbstractODEFunction}, u0,tspan,p=nothing, alg::AbstractForwardSensitivityAlgorithm = ForwardSensitivity(); kwargs...)

but in the explanation of the syntax, it accidently mentions a positional argument sensealg:

ODEForwardSensitivityProblem(f::SciMLBase.AbstractODEFunction,u0,
                             tspan,p=nothing,
                             sensealg::AbstractForwardSensitivityAlgorithm = ForwardSensitivity();
                             kwargs...)

Since sensealg is a keyword argument for the common solve interface and the direct adjoint sensitivity interface, users (i.e. me) may accidently and unfruitfully try to specify sensealg as a keyword argument in a call to ODEForwardSensitivityProblem.

Perhaps the positional argument alg, should be changed to a keyword argument sensealg in line with the remaining interfaces?

Best regards,
Marcus

@mschytt mschytt changed the title Consistently in sensealg arguments Consistency in sensealg arguments Aug 1, 2024
@ChrisRackauckas
Copy link
Member

Thanks for the report, it's not fixed with depwarns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants