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

Return nothing for getter methods that are not supported #217

Merged
merged 4 commits into from
Nov 9, 2020

Conversation

ueliwechsler
Copy link
Collaborator

closes #200

Following the proposal of @schillic, I implemented a default case for all getters that returns nothing if the method is not implemented. Additionally, I tried to give the unit tests for discrete and continuous systems more structure (which resulted in a large PR...sorry 😁)

As a by-product, I also added the supertype AbstractSystem to the SystemWithOutput type.

@schillic
Copy link
Member

Following the proposal of @schillic, I implemented a default case for all getters that returns nothing if the method is not implemented.

Great that everything is documented here. I would have said the opposite now 😄

@ueliwechsler
Copy link
Collaborator Author

What would you suggest now?

@schillic
Copy link
Member

I'm still fine with your solution, don't worry 😉

@ueliwechsler
Copy link
Collaborator Author

I am happy to hear that but this was not why I asked. Going through the comments and the code again let met realize that I was against using nothing some months ago and now, it felt really natural to implement.
But there are some drawbacks to using nothing from a user perspective. Therefore, I was wondering what your reasoning and solution would be now?

@mforets
Copy link
Member

mforets commented Oct 30, 2020

This is truly awesome @ueliwechsler, thanks for the contribution.

Concerning the related issue #180, we will have to add constructors that work with some unspecified fields. (I don't mind if we do this at the level of the @system and @ivp macro, or at the level of the constructors themselves.)

@mforets
Copy link
Member

mforets commented Oct 30, 2020

From the perspective of the ReachabilityAnalysis API, I think that being able to specify an @ivp(....) with a constraint on the input is a step forward (at the moment, if we want to say what's the constraint on U we also have to specify the constraint on the states).

@schillic
Copy link
Member

I was wondering what your reasoning and solution would be now?

I think I just got more used to undefined methods. But I still agree that for a new user, having a default definition is nicer. Since nothing makes sense here, I would prefer that over an error message, but not with a strong opinion.

@ueliwechsler
Copy link
Collaborator Author

Concerning the related issue #180, we will have to add constructors that work with some unspecified fields. (I don't mind if we do this at the level of the @system and @ivp macro, or at the level of the constructors themselves.)

I am again playing around with the @system macro ... trying to get a feeling for it.
I think it is absolutely sufficient to define it on the @system macro-level and always create a Constrained system.
Would nothing work as a default "Universe" type with respect to the ReachabilityAnalysis API?
Internally, we could extend the _in_stateset methods to return true if the set is nothing.

@mforets
Copy link
Member

mforets commented Nov 1, 2020

Would nothing work as a default "Universe" type with respect to the ReachabilityAnalysis API?

Yes, we can handle that easily by making a new dispatch on structs with a Nothing field, which should behave as if it was a LazySets.Universe.

In fact after parsing the user input, (linear) reachability algorithms normalize systems to only x' = Ax (LCS) or the canonical form x' = Ax + u (CLCCS) with state and input constraints on x and u respectively. This covers all relevant cases. The normalization step is currently implemented here. But this normalization would also be useful for another package. I think a good idea is to port such normalization to MathematicalSystems.jl.

@blegat
Copy link
Member

blegat commented Nov 2, 2020

There could be a way to get an error too

function well_chosen_function_name(func::Function, system, args...)
    ret = func(system, args...)
    if ret === nothing
        error("$func is not implemented by $(typeof(system)).")
    end
    return ret
end

So if someone is sure that statedim should be implemented and want a nice error if it is not because it does not handle nothing being returned in his code, he can call well_chosen_function_name(statedim, system)

As = [a][:,:]; Bs = [b][:,:]; Cs = [c]; Ds = [d][:,:]; Es = [e][:,:]



@testset "Continuous identity system" begin
Copy link
Member

Choose a reason for hiding this comment

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

I learned something new here (doc): You can nest @testsets! In case of no error, it will just print the outer test sets. In case of an error, it will print the inner test sets as well. I just wanted to share this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!
Yes, the nesting is nice, it makes the testing output so much neater.

@mforets
Copy link
Member

mforets commented Nov 9, 2020

Merging..

@mforets mforets merged commit d351924 into master Nov 9, 2020
@mforets mforets deleted the ueliwechsler/180 branch November 9, 2020 19:22
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 better error messages for getter function which are not supported
4 participants