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 @system macro #125

Merged
merged 76 commits into from
Jan 23, 2020
Merged

Add @system macro #125

merged 76 commits into from
Jan 23, 2020

Conversation

ueliwechsler
Copy link
Collaborator

@ueliwechsler ueliwechsler commented Dec 24, 2019

Closes #76.

@schillic
Copy link
Member

Does this close #76?

@ueliwechsler
Copy link
Collaborator Author

Yes, once it is done.

src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
@ueliwechsler
Copy link
Collaborator Author

Similar to the noise default value, I added a default value for input u which can be changed by adding input: u1 to the system definition.

@schillic
Copy link
Member

another try on making doctests work

You can let Documenter fix the doctests for you 😉

@ueliwechsler
Copy link
Collaborator Author

another try on making doctests work

You can let Documenter fix the doctests for you 😉

😂🙈 the more you know

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.

There are still some unresolved discussions here. Can you check if they can be closed?

src/macros.jl Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Show resolved Hide resolved
src/macros.jl Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
test/@system.jl Outdated Show resolved Hide resolved
test/@system.jl Outdated Show resolved Hide resolved
test/@system.jl Outdated Show resolved Hide resolved
test/@system.jl Outdated Show resolved Hide resolved
@ueliwechsler
Copy link
Collaborator Author

I tried to address the comments from @schillic as well as possible and updated the todo list #125 (comment).

I found some more "loopholes" in the definition and the documentation is not yet where it should be, but I think the behavior of the macro, in general, is as it is supposed to be. The requested or intended changes are either bug fixes or additional feature, but not breaking.

The remaining design decision that comes to my mind is if we want to have dim=1 as default case for affine systems with only one term such as x'=0 or x'=2x.

@mforets
Copy link
Member

mforets commented Jan 21, 2020

The remaining design decision that comes to my mind is if we want to have dim=1 as default case for affine systems with only one term such as x'=0 or x'=2x.

If the scalar differential equation x' = 2x can be written as @system x' = 2x, that's simple and intuitive. If we require to additionally write , dim=1, this adds one (small but) unnecessary level of verbosity. The argument dim allows one to specify a non-default value of the state-space dimension.

src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
Co-Authored-By: Christian Schilling <schillic@informatik.uni-freiburg.de>
@ueliwechsler
Copy link
Collaborator Author

ueliwechsler commented Jan 21, 2020

If the scalar differential equation x' = 2x can be written as @system x' = 2x, that's simple and intuitive

I agree with you. The question came up in a review comment.

The argument dim allows one to specify a non-default value of the state-space dimension.

Another point as part of a comment is having similar default value for black-box sytems, i.e. dim=(1,1,1) as default (see #125 (comment)). What is your opinion on that?

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.

Looks good from my side.

@mforets
Copy link
Member

mforets commented Jan 21, 2020

Another point as part of a comment is having similar default value for black-box sytems, i.e. dim=(1,1,1) as default (see #125 (comment)). What is your opinion on that?

In such systems, the dimension is part of the struct so providing that information seems reasonable. Moreover, how can one tell the dimension from x ' = f(x)? I can't think of an example where there could be an ambiguity. In x' = 2x, if x is a vector we can pass dim = n otherwise it is interpreted as a scalar.

@ueliwechsler
Copy link
Collaborator Author

I agree. So we keep the implementation as it is now, right?
Is there anything else to address for this PR?

@schillic
Copy link
Member

schillic commented Jan 21, 2020

Is there anything else to address for this PR?

There are two one unresolved discussions here and here.

@mforets mforets mentioned this pull request Jan 23, 2020
12 tasks
@mforets
Copy link
Member

mforets commented Jan 23, 2020

LGTM, great job!!! 🎉

Regarding the other tasks listed before, I opened #133.

@mforets mforets merged commit fe089ad into master Jan 23, 2020
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 @system macro
5 participants