-
-
Notifications
You must be signed in to change notification settings - Fork 100
Support non-Array AbstractVectors for AutoEnzyme hess and fgh! #1274
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
base: master
Are you sure you want to change the base?
Changes from all commits
e067321
7db3a5f
9b59807
cc12a1b
27c8188
bccf37d
6a2fb0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,20 @@ optprob.cons_h(H3, x0) | |
| optprob.lag_h(H4, x0, σ, μ) | ||
| @test H4 ≈ σ * H2 + μ[1] * H3[1] rtol = 1.0e-6 | ||
|
|
||
| # Test non-Vector AbstractVector (e.g. SubArray) for AutoEnzyme hess and fgh! | ||
| x_view = @view zeros(4)[1:2] | ||
| optprob_view = OptimizationBase.instantiate_function( | ||
| OptimizationFunction(rosenbrock, OptimizationBase.AutoEnzyme()), x_view, | ||
| OptimizationBase.AutoEnzyme(), nothing, 0, h = true, fgh = true | ||
| ) | ||
| H_view = Array{Float64}(undef, 2, 2) | ||
| G_view = Array{Float64}(undef, 2) | ||
| optprob_view.hess(H_view, x_view) | ||
| @test H1 == H_view | ||
| optprob_view.fgh(G_view, H_view, x_view) | ||
| @test G1 == G_view | ||
| @test H1 == H_view | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should test other things like GPUArrays, StaticArrays, ComponentArrays, etc.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll let you take over this PR then
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's fine. |
||
| G2 = Array{Float64}(undef, 2) | ||
| H2 = Array{Float64}(undef, 2, 2) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct, it's coercing into an array which for example would not be the correct behavior on GPUArrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the existing behavior here:
already does the conversion [we just need to match this for both primal+shadow]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well then that's the incorrect part that should get fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how? you cannot guarantee the primal/shadow are the same type [and index offsets] in a nice way that I can think of offhand otherwise (ofc longer term you can make a helper to adapt the array type, but that's a follow up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make_zero, Adapt.jl, ArrayInterface.restructure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of those work here, but I'll let you continue it