-
Notifications
You must be signed in to change notification settings - Fork 145
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
Added Functionality for FieldVectors (from StaticArrays) #307
Open
kylejbrown17
wants to merge
5
commits into
JuliaDiff:master
Choose a base branch
from
kylejbrown17:pull-request/fae6d6ce
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7d964c7
Gradient computation seems to be working for FieldVectors.
kylejbrown17 3f19cc3
Hessian computation working for FieldVectors
kylejbrown17 fae6d6c
Full functionality with type union of SArray and FieldVector
kylejbrown17 d953ba0
added tests for dispatching, mutable fieldvectors
zsunberg 7629717
fixed type issue in tests
zsunberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,17 +17,19 @@ end | |
################################### | ||
# vector mode function evaluation # | ||
################################### | ||
|
||
@generated function dualize(::Type{T}, x::SArray{S,V,D,N}) where {T,S,V,D,N} | ||
@generated function dualize(::Type{T}, x::Union{FieldVector, SArray}) where {T} | ||
N = length(x) | ||
V = eltype(x) | ||
dx = Expr(:tuple, [:(Dual{T}(x[$i], chunk, Val{$i}())) for i in 1:N]...) | ||
return quote | ||
chunk = Chunk{N}() | ||
chunk = Chunk{$N}() | ||
$(Expr(:meta, :inline)) | ||
return SArray{S}($(dx)) | ||
# return SArray{S}($(dx)) | ||
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. We should delete this commented out line. 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. Ah, sorry about that. Not sure why I left that sitting in there. |
||
similar_type($x,Dual{T,$V,$N})($(dx)) | ||
end | ||
end | ||
|
||
@inline static_dual_eval(::Type{T}, f, x::SArray) where {T} = f(dualize(T, x)) | ||
@inline static_dual_eval(::Type{T}, f, x::Union{FieldVector, SArray}) where {T} = f(dualize(T, x)) | ||
|
||
function vector_mode_dual_eval(f, x, cfg::Union{JacobianConfig,GradientConfig}) | ||
xdual = cfg.duals | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The type constraint on
x
allows this (and the other similar changes in this PR) to technically work, since we can probably assume that no upstream code will re-overloadlength
oreltype
forUnion{FieldVector, SArray}
.However, it's a bit sketchy...it makes it easier for bugs to accidentally be introduced in the future via seemingly innocent changes (e.g. loosening the type constraint on
x
).Would it be hard to just pull these out of the type parameter directly instead?
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.
It seems like calling
length
andeltype
makes this more straightforward to extend to additional types. @zsunberg and I decided to retrieveN
andV
vialength
andeltype
because it was already quite complicated to try to extract them directly from the type parameters.I've tried a few different variations on this:
Everything I've tried fails when called on an instance of
FieldVector
, although most of the approaches seem to work onSVector
. I'm not 100% sure what you mean by "loosen the type constraint", but it seems like that would cause an even bigger headache if we want to extract the parameters this way.If we tried to extract type parameters directly within the function body, we'd need a separate case for each type (because
V
andN
are in different places for each type):Either of the above ways requires more customization than simply calling
length
andeltype
. I think this way is more stable. It should work for vector types for whichlength
andeltype
are defined.Another option is to have two different function definitions in every case where we need
V
andN
.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 think you might misunderstand my concern, which is my bad - I didn't explain the issue directly enough. If this were normal code, then of course I'd be fine with
length
andeltype
getting used here.However, this code is being called in an
@generated
thunk, and in general, calling overloadable methods inside of@generated
function thunks is something that should be done with care, because downstream code can cause the generator to be "invalidated" in ways the compiler can't easily predict. Such code can cause world-age errors or silently incorrect method lookups, and are very hard for end-users to fix/debug/understand because they can depend on precompilation settings, code load order, etc. Thus, we want to be very careful about potentially introducing these kinds of bugs.In this case, since
FieldVector
is an abstract type, so I could subtype it and overloadlength
andeltype
on that subtype. For the sake of example, let's call this subtypeMyFieldVectorSubtype
. If a user loaded ForwardDiff, calleddualize
on some other<:FieldVector
, then loaded the code that definesFieldVectorSubtype
, then calleddualize
on an instance ofMyFieldVectorSubtype
, the compiler could possibly throw a world-age error (most likely), silently call the wrong methods, run into some other compilation error, etc.While this may seem like a niche class of bugs, they can crop up in practice quite regularly; for one ForwardDiff-related instance of such a bug, see JuliaArrays/StaticArrays.jl#316.
By the way, I really appreciate this PR, and am excited for this feature!
EDIT: With the above in mind, it might be sufficient to just define some internal helper functions, and leave a note in the code, e.g.
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.
Ah ok, yeah, we didn't understand the specific concerns with @generated
What about
?
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.
(we didn't know about the <:Any syntax in the type parameters)
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.
If that doesn't work, I think two dualize functions would be a little simpler than _length and _eltype
EDIT: never mind - I didn't see that this is scattered throughout the other code
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.
@jrevels, thanks for clarifying! This makes more sense. We'll incorporate the changes you suggested and get back to you.