-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add conversion to from FastDifferentiation.jl format #953
Conversation
|
||
@assert !(typeof(sym_node) <: Symbolics.Arr) "Differentiation of expressions involving arrays and array variables is not yet supported." | ||
if SymbolicUtils.istree(Symbolics.unwrap(sym_node)) | ||
@assert !SymbolicUtils.issym(SymbolicUtils.operation(Symbolics.unwrap(sym_node))) "expressions of the form `q(t)` not yet supported." |
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.
Pretty much all use cases would use a x(t)
symbol, but they act the same in most Jacobians as x
. Is there a reason why it wouldn't be supported? It would just be a simple conversion.
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 have support for expressions of the form q(t) but there is a bug in the code that causes these types of expressions to sometimes fail so for now it is not allowed. It is in the queue to be fixed.
When there is an FD term dq(t)/dt = q̇(t) what do you want me to return as the value of q̇(t)? Something like this: D(t)(q)?
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 value of q̇(t)
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.
Did not understand your answer. If somebody does this in Symbolics:
using Symbolics
@variables t q(t)
fd_jacobian([q],[t])
then (when the bug is fixed and FD properly handles forms like q(t)) FD will compute a term q̇
. This is represented with a special node type in FD. When q̇
is converted from FD to Symbolics form should it be Differential(t)(q))
?
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.
Differential(t)(q(t))
it's a little weird but in this case, specifically @variables t q(t)
binds q
to q(t)
.
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 will need to dynamically construct q(t)
. Is there a function to do this? I poked around in the Symbolics.jl src and found this
Line 472 in ee3e491
function variable(name, idx...; T=Real) |
q(t)
. If it is could you give me an example of how it should be called?
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.
Maybe something like this?
function f(q::Symbol,t::Symbol)
tmp = @variables $q($t)
return(tmp[1])
end
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #953 +/- ##
=========================================
- Coverage 8.35% 8.17% -0.19%
=========================================
Files 26 27 +1
Lines 3266 3341 +75
=========================================
Hits 273 273
- Misses 2993 3068 +75
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Looking great, can merge once q(t)
works.
else | ||
return x * S(T, m - 1, x, y) + y * C(T, m - 1, x, y) | ||
end | ||
end |
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.
nice 👍
The second tuple term will be `differentiation_variables` converted to `FastDifferentiation` form. | ||
This tuple can be passed to `fd_make_function` to create an efficient executable.""" | ||
fd_jacobian(symbolics_function::AbstractArray{Symbolics.Num}, differentiation_variables::AbstractVector{Symbolics.Num}; fast_differentiation=false) = remap(FD.jacobian, symbolics_function, differentiation_variables, fast_differentiation) | ||
export fd_jacobian |
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'd love for this to replace jacobian
, but not until q(t)
starts working!
@brianguenter @shashi curious about the expected timeline for this? This would be very useful in Optimization.jl so I am quite excited about it! I can help out if there's something blocking this that doesn't need an expert of two the packages? |
@Vaibhavdixit02 @sashi It's waiting on me fixing a bug that requires a detailed understanding of the internals of the FD algorithms, which are not well documented. I wouldn't stop you from trying but it would be a deep dive. I had started on this last week before I went on vacation. Will be getting back into it this week. How long it will take is unpredictable. It's gnarly code and the error only occurs with big complex expression graphs, which makes debugging more difficult and slow. |
Okay! Thanks for the update 🙌 |
Draft pull request to see if this is along the lines of what you were thinking.