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 better error messages for getter function which are not supported #200

Closed
ueliwechsler opened this issue Mar 14, 2020 · 5 comments · Fixed by #217
Closed

Add better error messages for getter function which are not supported #200

ueliwechsler opened this issue Mar 14, 2020 · 5 comments · Fixed by #217

Comments

@ueliwechsler
Copy link
Collaborator

It's a small thing but consider an unconstrained system

sys = @system(x' = Ax + Bu)

and try to access a nonexisting field, the error message could be improved.

stateset(a)
ERROR: MethodError: no method matching stateset(::LinearControlDiscreteSystem{Float64,Array{Float64,2},Array{Float64,2}})
Closest candidates are:
  stateset(::ConstrainedContinuousIdentitySystem) at C:\Users\wueli\.julia\dev\MathematicalSystems\src\systems.jl:71
  stateset(::ConstrainedDiscreteIdentitySystem) at C:\Users\wueli\.julia\dev\MathematicalSystems\src\systems.jl:71
  stateset(::ConstrainedLinearContinuousSystem) at C:\Users\wueli\.julia\dev\MathematicalSystems\src\systems.jl:327
  ...
Stacktrace:
 [1] top-level scope at none:0

For example we could return:

ERROR: the state set cannot be accessed for a system of  type `LinearControlDiscreteSystem` 
@mforets
Copy link
Member

mforets commented Mar 14, 2020

yes, the current error message doesn't say too much and it can be improved, more explicitly ".. the state set and the input constraints must be specified for this type of system". On the other hand, i consider this only a temporary fix that we should be able to handle after #180, possibly filling such fields with nothing (again this can be changed later but it is a good step forward)

@ueliwechsler
Copy link
Collaborator Author

Another approach would be to return nothing for all types where stateset, noiseset etc are not defined. Which is preferable?

@mforets
Copy link
Member

mforets commented Mar 25, 2020

Which is preferable?

I don't know in general, but for example the approach in ReachabilityAnalysis is:

system  = @system(x' = Ax) # user defines a system
# ...  user associates this system with an initial-value problem and calls solve ...
# internally:
normalize(system) # returns a system with Universe as state constraints
discretize(..) ... reach(..) ...

So whenever one hits a system whose stateset is not defined, normalize (defined here) attaches a universal stateset. After normalization, algorithms can rely on having the stateset method to work and it's easier to write a generic algorithm.

@mforets
Copy link
Member

mforets commented Mar 25, 2020

for this issue, i think that improving the error message is fine. the code could be simplified using an interface (or traits) but there is no much done about that yet in this library.

@schillic
Copy link
Member

On the other hand, i consider this only a temporary fix that we should be able to handle after #180, possibly filling such fields with nothing (again this can be changed later but it is a good step forward)

A LinearControlDiscreteSystem (as in the OP) will never have state constraints, even after #180. This issue is only about an undefined method.

Another approach would be to return nothing for all types where stateset, noiseset etc are not defined.

I prefer this behavior. Receiving nothing for something that does not exist is natural and easier to work with than an error message saying that you are asking for something that does not exist for a specific system instance. You can always override this behavior to return your favorite universe representation of course, but the default behavior with nothing sounds reasonable.

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 a pull request may close this issue.

3 participants