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 function to return vector_field of continuous system #201

Merged
merged 22 commits into from
Oct 29, 2020

Conversation

ueliwechsler
Copy link
Collaborator

@ueliwechsler ueliwechsler commented Mar 14, 2020

Closes #160.

  • Adds method vector_field to systems of type AbstractContinuousSystem.
  • Rewrites successor method to make it more generic (no need to add a new method for every new type)
  • add getter function mapping for retrieving the parametric function:p or blackbox function :f. (function name is WIP though)
  • add isblackbox trait
  • add Tests and docstring for VectorField type

@mforets
Copy link
Member

mforets commented Apr 15, 2020

Is this still WIP?

@ueliwechsler
Copy link
Collaborator Author

Sorry, I did not have that much time to work on this PR in the last weeks.

The only thing that is left to do is the following:

  • add Tests and docstring for VectorField type
  • Export VectorField and evaluate?

I am not really used to the wording of continuous systems and, to be honest, wording in general 😅. If you could provide some help, that would be really great.

But apart from adding documentation to the VectorField part here

struct VectorField{T}
field::T
end
# function-like evaluation
@inline function (V::VectorField)(args...)
evaluate(V, args...)
end
function evaluate(V::VectorField, args...)
return V.field(args...)
end
function VectorField(sys::AbstractContinuousSystem)
if inputdim(sys) == 0 && noisedim(sys) == 0
field = (x) -> vector_field(sys, x)
elseif inputdim(sys) == 0 || noisedim(sys) == 0
field = (x, u) -> vector_field(sys, x, u)
else
field = (x, u, w) -> vector_field(sys, x, u, w)
end
return VectorField(field)
end
and possibly adding a Test for VectorField this PR is fine. And should not be WIP.

@mforets
Copy link
Member

mforets commented Apr 19, 2020

Alright, thanks for the write-up. I'm enthusiastic about this PR as it gives access to vector_field. I'll think about naming suggestions and reply back.

@ueliwechsler ueliwechsler changed the title WIP: Add function to return vector_field of continuous system dd function to return vector_field of continuous system Oct 18, 2020
@ueliwechsler ueliwechsler changed the title dd function to return vector_field of continuous system Add function to return vector_field of continuous system Oct 18, 2020
@ueliwechsler
Copy link
Collaborator Author

Finally, I finished my work on vector_field and successor rewriting.

@schillic
Copy link
Member

Can you split up the file src/instantiate.jl? At least I would not look for successor or VectorField in there. If there is something in common for all the content, a different name would be better.

@ueliwechsler
Copy link
Collaborator Author

Sure. I can create a file vector_field.jl and successor.jl for the corresponding code and keep the _instantiate "helper" methods in the isntantiate.jl file. I could move these three files into folder too. Any good namesuggestions?

@schillic
Copy link
Member

Sounds good. I do not have suggestions for a folder name, but I also think we do not need a folder.

Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
The VectorField struct is not tested I think.

@ueliwechsler
Copy link
Collaborator Author

I added the tests. Should be fine now.

@ueliwechsler
Copy link
Collaborator Author

I haven't done this in a while. Can I just merge or do I have to rebase or similar?
I just remember, that the last time I tried, it ended for me in confusion and chaos😅.

@schillic
Copy link
Member

Let's wait if @blegat has something to say. If you have the rights to do so, you can just merge (no conflict here).

@mforets
Copy link
Member

mforets commented Oct 29, 2020

Merging!

@mforets mforets merged commit 52af047 into master Oct 29, 2020
@mforets mforets deleted the ueliwechsler/160 branch October 29, 2020 14:40
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.

Add function to return gradient of continuous system
3 participants