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 warnings for functionality that exists but is optional #632

Closed
mforets opened this issue Sep 18, 2018 · 10 comments
Closed

Add warnings for functionality that exists but is optional #632

mforets opened this issue Sep 18, 2018 · 10 comments
Labels
duplicate 2️⃣ This issue or pull request already exists

Comments

@mforets
Copy link
Member

mforets commented Sep 18, 2018

The current behavior for functions which depend on an optional package does not say much:

julia> using LazySets
INFO: Recompiling stale cache file /Users/forets/.julia/lib/v0.6/LazySets.ji for module LazySets.

julia> V1 =VPolytope([rand(2) for i in 1:10]);

julia> V2 =VPolytope([rand(2) for i in 1:10]);

julia> intersection(V1, V2)
ERROR: MethodError: no method matching intersection(::LazySets.VPolytope{Float64}, ::LazySets.VPolytope{Float64})

A possibility is to export the functionality that exists but with a warning that one needs Polyhedra for it to work, i mean adding:

function intersection(P1::VPolytope{N},
                      P2::VPolytope{N}) where {N}
    warn("you need to load `Polyhedra` library for this operation")
end

Then the behavior is the following:

julia> using LazySets
INFO: Recompiling stale cache file /Users/forets/.julia/lib/v0.6/LazySets.ji for module LazySets.

julia> VPolytope([rand(2) for i in 1:10]);

julia> V1 =VPolytope([rand(2) for i in 1:10]);

julia> V2 =VPolytope([rand(2) for i in 1:10]);

julia> intersection(V1, V2)
WARNING: you need to load `Polyhedra` library for this operation

julia> using Polyhedra

julia> intersection(V1, V2)
LazySets.VPolytope{Float64}(Array{Float64,1}[[0.27792, 0.913931], [0.0352844, 0.873993], [0.0311079, 0.837181], [0.639983, 0.133231], [0.0301204, 0.665349], [0.0772336, 0.128488], [0.911773,
0.535153], [0.894588, 0.736061]])
@mforets mforets changed the title Add warnings for warnings for functionality that exists but is optional Add warnings for functionality that exists but is optional Sep 18, 2018
@mforets
Copy link
Member Author

mforets commented Sep 18, 2018

A more radical but i think interesting approach is to wrap the class of "concrete polytopes", or some of their functionality, into a module ConcretePolyhedra. This module will run the command using Polyhedra and the stuff it needs to interact with LazySets (no more require).

@mforets mforets added the discussion 🗣️ Requires human input label Sep 18, 2018
@schillic
Copy link
Member

schillic commented Sep 18, 2018

Adding the function, I would have expected that you see a warning that the function is redefined when loading Polyhedra.
Alternatively, we could add an if !polyhedraloaded warn("warning...") end at the beginning of each function, where polyhedraloaded is the code to check if the package is loaded (I would have to look it up).

The second option sounds good, but it will bring problems like: Those types are not available in pure LazySets anymore, so all code that involves XPolytopes will have to go.

@mforets
Copy link
Member Author

mforets commented Sep 18, 2018

Adding the function, I would have expected that you see a warning that the function is redefined when loading Polyhedra.

in v0.6.4 it worked like that above, without warning about the redefinition. i think that this method looks a bit untidy, though. but i don't like having MethodError when the functionality actually exists.

@mforets
Copy link
Member Author

mforets commented Sep 18, 2018

The second option sounds good, but it will bring problems like: Those types are not available in pure LazySets anymore, so all code that involves XPolytopes will have to go.

The types can stay in lazysets, i was expecting it to be possible to pull out only the parts inside function load_polyhedra_abstractpolytope().

@schillic
Copy link
Member

Hm, then I do not understand that proposal (sorry).

@mforets
Copy link
Member Author

mforets commented Sep 18, 2018

It is only the methods inside load_polyhedra_abstractpolytope() that are hidden before using Polyhedra. One can create the objects, calculate set membership or even support vectors without Polyhedra. I somehow wanted to "uncover" those methods that are hidden.

@schillic
Copy link
Member

It is still not clear to me. Either

  • the module is optional, and then you will still see the MethodError before loading it (right?), or
  • the module is not optional, and using Polyhedra is not good to hard-code then.

@mforets
Copy link
Member Author

mforets commented Sep 18, 2018

Well you are right, if the module is optional, nothing changes, one still will see the method error.

@schillic schillic added the usability 🖱️ Simplifies the usage or interface label Sep 18, 2018
@schillic
Copy link
Member

schillic commented Sep 18, 2018

Your initial proposal does not print a warning for me as well. We could go that way from my side.
Disadvantages:

  • You have to manually add methods where you want to print this warning.
  • You cannot have the default parameter backend=default_polyhedra_backend(N) in these new methods, so a user who wants to use the call with a backend will again get a MethodError.

@schillic schillic added duplicate 2️⃣ This issue or pull request already exists and removed discussion 🗣️ Requires human input usability 🖱️ Simplifies the usage or interface labels Jun 20, 2019
@schillic
Copy link
Member

I close this in favor of #1446.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate 2️⃣ This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants