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.

Sign up for GitHub

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

Jump to bottom

Default algorithm should have checks for Adjoints limitations #1116

Closed
prbzrg opened this issue Sep 19, 2024 · 4 comments · Fixed by #1145
Closed

Default algorithm should have checks for Adjoints limitations #1116

prbzrg opened this issue Sep 19, 2024 · 4 comments · Fixed by #1145
Labels
bug

Comments

@prbzrg
Copy link
Member

prbzrg commented Sep 19, 2024

Describe the bug 🐞

I'm getting

ERROR: LoadError: autojacvec choice SciMLSensitivity.TrackerVJP(false) is not supported by GaussAdjoint

When I don't specify sensealg.
I think the default algorithm for selecting sensealg doesn't check for limitation of each Adjoint.
@prbzrg prbzrg added the bug label Sep 19, 2024
@ChrisRackauckas
Copy link
Member

What's an MWE for this?

@ChrisRackauckas
Copy link
Member

There is no default to TrackerVJP, so it cannot be the default algorithm?

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2024

In the function automatic_sensealg_choice in the file:
https://github.com/SciML/SciMLSensitivity.jl/blob/master/src/concrete_solve.jl
choosing vjp happen before choosing Adjoint and passing vjp to it
the selection is separated and independent of each other.

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2024

It's possible that TrackerVJP get selected

SciMLSensitivity.jl/src/concrete_solve.jl

Line 167 in 4847a87

TrackerVJP()

and then we pass it to GaussAdjoint

SciMLSensitivity.jl/src/concrete_solve.jl

Line 198 in 4847a87

GaussAdjoint(autojacvec = vjp)

ChrisRackauckas added a commit that referenced this issue Nov 9, 2024
@ChrisRackauckas
Don't allow picking GaussAdjoint with TrackerVJP in default alg
8ec9647 
Fixes #1116 . I don't have a case that actually hits this though, no MWE provided, so I can't add a test but it's at least a thing that needs to be done.
@ChrisRackauckas ChrisRackauckas mentioned this issue Nov 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't allow picking GaussAdjoint with TrackerVJP in default alg SciML/SciMLSensitivity.jl
3 participants
@ChrisRackauckas @prbzrg and others