Closed krooken closed 3 years ago
@krooken Let us know if you want to add an e-mail to the commit.
@johnyf I suggest we give @krooken a few days, then merge.
I've added the new email to my GitHub account, so it should be good to go now.
Thank you for the update. Merged as f981f434a270ee005076b9c534383aeaba314f40.
Thank you for this change. It looks good to me.
This error was introduced in https://github.com/tulip-control/polytope/commit/caf8c61655ea2c9c101fe29867a24cc0028bfd13#diff-7b1ac28f438b876806216e8120a046db34b027f8d1c02a0aca1ceac1e2a543efR109. The change in this pull request agrees with the initial version of those lines: https://github.com/tulip-control/polytope/commit/125d8da285d473ec2ad15692fe4cac888c27a423#diff-7b1ac28f438b876806216e8120a046db34b027f8d1c02a0aca1ceac1e2a543efR120.
Relevant remarks:
lp_solver
(which could have beenNone
https://github.com/tulip-control/polytope/blob/125d8da285d473ec2ad15692fe4cac888c27a423/polytope/esp.py#L62) was passed as argumentsolver
to the functionpolytope.solvers.lp
: https://github.com/tulip-control/polytope/blob/125d8da285d473ec2ad15692fe4cac888c27a423/polytope/esp.py#L155-L157 However, later changes made all calls to the functionpolytope.solvers.lpsolve
from the modulepolytope.esp
pass as argumentsolver
the value'glpk'
, for example: https://github.com/tulip-control/polytope/blob/b11c5cecf00ae4c8fc5021289460b4cf9cd971a8/polytope/esp.py#L144 This ensures that in case GLPK is not available, then all calls from moduleesp
will fail by raising the exception: https://github.com/tulip-control/polytope/blob/b11c5cecf00ae4c8fc5021289460b4cf9cd971a8/polytope/solvers.py#L152-L155 So the check in functionpolytope.esp.esp
duplicates that check in the functionpolytope.solvers._assert_have_solver
. Nonetheless, this check may be worth keeping, in order to print a message that explains that GLPK is a requirement of the modulepolytope.esp
(instead of having the user wonder why the exception was raised, if they did not request GLPK in their code).polytope.esp.esp
raises anException
: https://github.com/tulip-control/polytope/blob/b11c5cecf00ae4c8fc5021289460b4cf9cd971a8/polytope/esp.py#L110 whereas the functionpolytope.solvers._assert_have_solver
raises aRuntimeError
: https://github.com/tulip-control/polytope/blob/b11c5cecf00ae4c8fc5021289460b4cf9cd971a8/polytope/solvers.py#L152 The message of commit caf8c61655ea2c9c101fe29867a24cc0028bfd13 describes the rationale for choosingRuntimeError
instead ofImportError
. Based on the documentation ofRuntimeError
, it may be more suitable to raise aRuntimeError
instead of anException
when checking within the functionpolytope.esp.esp
whether GLPK is available.Before merging, I would like to note that GitHub does not appear to associate the commit 722ff00b805d86e864c9bfcfec3229032fd04586 with any GitHub account. If it is intended to associate the commit with your account, then please consider either:
If no email-related change is needed, then I will merge this pull request.