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

Add conversion to from FastDifferentiation.jl format #953

Closed
wants to merge 2 commits into from
Closed

Add conversion to from FastDifferentiation.jl format #953

wants to merge 2 commits into from

Conversation

brianguenter
Copy link

Draft pull request to see if this is along the lines of what you were thinking.


@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."
Copy link
Member

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.

Copy link
Author

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)?

Copy link
Member

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)

Copy link
Author

@brianguenter brianguenter Aug 13, 2023

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 . This is represented with a special node type in FD. When is converted from FD to Symbolics form should it be Differential(t)(q))?

Copy link
Member

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).

Copy link
Author

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

function variable(name, idx...; T=Real)
but this doesn't look like the right function to create a q(t). If it is could you give me an example of how it should be called?

Copy link
Author

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-commenter
Copy link

codecov-commenter commented Aug 12, 2023

Codecov Report

Merging #953 (7924165) into master (ee3e491) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

❗ 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     
Files Changed Coverage Δ
src/fdconversion.jl 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@shashi shashi left a 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
Copy link
Member

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
Copy link
Member

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!

@Vaibhavdixit02
Copy link
Collaborator

@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?

@brianguenter
Copy link
Author

@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.

@Vaibhavdixit02
Copy link
Collaborator

Okay! Thanks for the update 🙌

@brianguenter brianguenter closed this by deleting the head repository May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants