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

@system: error handling for variable names #155

Closed
wants to merge 2 commits into from

Conversation

ueliwechsler
Copy link
Collaborator

Complete an element of checklist in #133.

I am not sure about this point on the checklist. It helps for error handling in general, but in some cases, e.g. if you want to name your state u or w it would make the macro more verbose (see tests).

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.

I agree that @system(w' = Aw + Bu) working automatically would be preferred, but that is more complicated. This PR is an improvement already.

@mforets mforets mentioned this pull request Feb 1, 2020
12 tasks
@mforets
Copy link
Member

mforets commented Feb 3, 2020

Sorry, i don't understand the purpose of this change and the purpose of this item list, ensure that state≠input≠noise. Shouldn't we consider @test @system(x' = Ax + Bw, input:w, noise:w) a user error? Even if we want to check it, can't we do it without modifying the existing tests? (i mean for example, that @system(w' = Aw + Bu) still works? In the case the answer is that it is very hard, do we have a strategy in the longer run?

@schillic
Copy link
Member

schillic commented Feb 3, 2020

i mean for example, that @system(w' = Aw + Bu) still works?

Yes, that is what I said above, but that is more complicated. The current macro (in master) expects a certain variable name for state/input/noise. If you mix those names, the behavior is probably not what you would expect.

Shouldn't we consider @system(x' = Ax + Bw, input:w, noise:w) a user error?

Right, and this PR prints an error message instead of doing something unexpected. I find that better than the version in master, at least until we support that feature.

@ueliwechsler
Copy link
Collaborator Author

Sorry, i don't understand the purpose of this change and the purpose of this item list

The purpose is to prevent the user from doing things that could have arbitrary behaviour such as having both input and noise defined as w, e.g. @system(x'=Ax+Bw, input:w). In this case, it is not well defined what the macro does (from looking at the documentation alone).

Even if we want to check it, can't we do it without modifying the existing tests?

I think we should have these checks, it improves the quality, but we could do them more intelligent.
As an alternative method to check the input, we could apply the checks only if the user has defined a new variable name, i,e:

# we do not check for identical variable names
@system(w'=Aw+Bx)
# we do check for identical variable names because the user changed the bindings
@system(x'=Ax+Bw, input:w)

@mforets
Copy link
Member

mforets commented Feb 10, 2020

Ok, thanks. I guess what you are saying is to verify if the state_var, input_var or noise_var have been set, maybe along these lines:

    default_state_var = :x
    default_input_var = :u
    default_noise_var = :w

    state_var = nothing
    input_var = nothing
    noise_var = nothing
... 
# if state_var == nothing then it hasn't been set
# do consistency checks
state_var = state_var == nothing ? default_state_var
...

@ueliwechsler
Copy link
Collaborator Author

I will close this PR, since it is replaced by #164

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.

4 participants