Return type of eigenvalues from lqr and lqe not consistent #439

Activity

1. sawyerbfuller commented on Jul 29, 2020

   

   sawyerbfuller

   on Jul 29, 2020 via email

   Contributor

   More actions

   I am not sure what the convention is here but I saw elsewhere that there were functions that returned a matrix or an array depending on that setting, using _ssmatrix. So my take is that Lqr should probably do the same. S

   On Tue, Jul 28, 2020 at 5:49 PM Ben Greiner ***@***.***> wrote: This came up while pytestifying statefbk_test.py (#438 <#438>) lqr() will always return E as (nstates, ) array https://github.com/python-control/python-control/blob/d3142ff24c7af5e2c0843fd16d0e1be82b322bb9/control/statefbk.py#L444-L446 lqe() returns _ssmatrix(E) which is a (nstates, ) array or (1, nstates) matrix depending on config.defaults.use_numpy_matrix() https://github.com/python-control/python-control/blob/d3142ff24c7af5e2c0843fd16d0e1be82b322bb9/control/statefbk.py#L290 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#439>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSOF6TGZCDRQP5I2WBDR55WYBANCNFSM4PLCB64A> .

   -- Sawyer Fuller Assistant Professor of Mechanical Engineering University of Washington http://faculty.washington.edu/~minster/ (Typed with my thumbs)
2. bnavigator commented on Jul 29, 2020

   

   bnavigator

   on Jul 29, 2020

   ContributorAuthor

   More actions

   I agree :)
3. bnavigator commented on Aug 16, 2020

   

   bnavigator

   on Aug 16, 2020

   ContributorAuthor

   More actions

   See also #418
4. murrayrm

   added this to the 0.8.4 milestone on Dec 22, 2020
5. murrayrm

   self-assigned this

   on Dec 22, 2020
6. murrayrm commented on Dec 24, 2020

   

   murrayrm

   on Dec 24, 2020

   Member

   More actions

   I'd like to clean this up before 0.8.4 release, but there are a few issues that could break users' code:

   • lqr returns _ssmatrix(K), _ssmatrix(S), E, for which the first return value is the gain matrix (2D array or matrix, depending on config settings), the second return value is the solution to the Riccati equation (2D array or matrix), and the third value is a 1D array (list of closed loop eigenvalues).
   • lqe returns _ssmatrix(K), _ssmatrix(S), _ssmatrix(E), which forces the third argument to be a 2D array.
   • care and dare return _ssmatrix(X), w[:n], _ssmatrix(G), for which the first return value is the solution to the Riccati equation (2D array or matrix), the second return value is a list of eigenvalues (1D array), and the third argument is the gain matrix (2D array or matrix).

   I think we should make the following change in 0.8.4:

   • Change return values for lqe to match lqr

   This will require an updated unit test (as noted above) and could break some users' code. But the fix to user code is simple (convert to 2D array) and so I think it is OK for 0.8.4.

   I think we might want to consider whether to make changes to care and dare in 0.9.0:

   • We can leave things the same with the argument that care and dare are returning solutions to the Riccati equation as their primary usage => that solution should be first. The lqr and lqe functions call care or dare, but are returning controllers as their primary usage => OK for the order to be different.

   • We can change the order of the return values for care and dare to match lqr and lqe.

   My current feeling is that we leave care and dare alone. Thoughts?

   👍React with 👍1Reacted by Ben Greiner
7. sawyerbfuller commented on Dec 24, 2020

   

   sawyerbfuller

   on Dec 24, 2020 via email

   Contributor

   More actions

   Agree with richards’ take that ok for care and dare to have different return order. Off the top of my head I think matlab might do the same. Re lqe return argument - I think I made a mistake when I originally wrote it to naively turn E into an ssmatrix. Agree it should not have been. S

   On Thu, Dec 24, 2020 at 9:37 AM Richard Murray ***@***.***> wrote: I'd like to clean this up before 0.8.4 release, but there are a few issues that could break users' code: - lqr returns _ssmatrix(K), _ssmatrix(S), E, for which the first return value is the gain matrix (2D array or matrix, depending on config settings), the second return value is the solution to the Riccati equation (2D array or matrix), and the third value is a 1D array (list of closed loop eigenvalues). - lqe returns _ssmatrix(K), _ssmatrix(S), _ssmatrix(E), which forces the third argument to be a 2D array. - care and dare return _ssmatrix(X), w[:n], _ssmatrix(G), for which the first return value is the solution to the Riccati equation (2D array or matrix), the second return value is a list of eigenvalues (1D array), and the third argument is the gain matrix (2D array or matrix). I think we should make the following change in 0.8.4: - Change return values for lqe to match lqr This will require an updated unit test (as noted above) and could break some users' code. But the fix to user code is simple (convert to 2D array) and so I think it is OK for 0.8.4. I think we might want to consider whether to make changes to care and dare in 0.9.0: - We can leave things the same with the argument that care and dare are returning solutions to the Riccati equation as their primary usage => that solution should be first. The lqr and lqe functions call care or dare, but are returning controllers as their primary usage => OK for the order to be different. - We can change the order of the return values for care and dare to match lqr and lqe. My current feeling is that we leave care and dare alone. Thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSLASGB633QJ46ETPLTSWN3ZFANCNFSM4PLCB64A> .

   -- Sawyer Fuller Assistant Professor of Mechanical Engineering Adjunct, Paul G Allen School of Computer Science University of Washington http://faculty.washington.edu/~minster/ (Typed with my thumbs)
8. murrayrm commented on Dec 26, 2020

   

   murrayrm

   on Dec 26, 2020

   Member

   More actions

   Fixed in #477.
9. murrayrm

   closed this as completedon Dec 26, 2020