{"action":"created","issue":{"url":"https://api.github.com/repos/scipy/scipy/issues/4295","labels_url":"https://api.github.com/repos/scipy/scipy/issues/4295/labels{/name}","comments_url":"https://api.github.com/repos/scipy/scipy/issues/4295/comments","events_url":"https://api.github.com/repos/scipy/scipy/issues/4295/events","html_url":"https://github.com/scipy/scipy/pull/4295","id":52461396,"number":4295,"title":"Pole placement","user":{"login":"I--P","id":8004855,"avatar_url":"https://avatars.githubusercontent.com/u/8004855?v=3","gravatar_id":"","url":"https://api.github.com/users/I--P","html_url":"https://github.com/I--P","followers_url":"https://api.github.com/users/I--P/followers","following_url":"https://api.github.com/users/I--P/following{/other_user}","gists_url":"https://api.github.com/users/I--P/gists{/gist_id}","starred_url":"https://api.github.com/users/I--P/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/I--P/subscriptions","organizations_url":"https://api.github.com/users/I--P/orgs","repos_url":"https://api.github.com/users/I--P/repos","events_url":"https://api.github.com/users/I--P/events{/privacy}","received_events_url":"https://api.github.com/users/I--P/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/scipy/scipy/labels/enhancement","name":"enhancement","color":"bfe5bf"},{"url":"https://api.github.com/repos/scipy/scipy/labels/scipy.signal","name":"scipy.signal","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":52,"created_at":"2014-12-19T09:06:22Z","updated_at":"2015-02-04T16:29:46Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/scipy/scipy/pulls/4295","html_url":"https://github.com/scipy/scipy/pull/4295","diff_url":"https://github.com/scipy/scipy/pull/4295.diff","patch_url":"https://github.com/scipy/scipy/pull/4295.patch"},"body":"Please find in this PR two pole placement algorithms for linear systems.\r\n\r\nI started a code review topic for this on scipy-dev a few days ago and nobody raised any objections so I'm issuing a pull request. Please find below a cut'n paste of the code review request with explanations on what the code does and my concerns regarding licensing issues (it is really important to dig into these imho).\r\n\r\nYou will see that there are a lot of comments in the code, I hope they are not too verbose but I thought it was important to motivate every choice I made as the original publications are sometimes sloppy on details (see for example the choice of the initial transfer matrix X or what I do when an \"improvement\" on the transfer matrix yields a null vector, lines 1481 and 1175).\r\n\r\n--- CUT'N PASTE STARTS HERE ---\r\n\r\nAs said a few days ago there are two algorithms, KNV method 0 (KNV0) and \r\nYang Tits (YT). See code for urls to the reference papers.\r\n\r\nMotivations for doing this in pure Python rather than linking some old \r\nfortran code:\r\n\r\nI was in need of a 100% Python dependency free (except Numpy of course) \r\npole placement function, integrating it into scipy was not my primary \r\ngoal. Fortran code used for decades will for sure be faster and bug free \r\nbut it was not an option.\r\n\r\nRegarding KNV0:\r\n\r\n- Licensing issues: I chose this method before knowing Matlab used it \r\ntoo. I did read some old matlab source code (circa 2001 release iirc) at \r\nthe beginning of my work I won't lie on that, after Ralf Gommers warned \r\nme it was not possible to port Matlab code I used my knowledge of their \r\ncode to stay as far as possible from what they did (the algorithm gives \r\nsome degrees of freedom). Given the current status of my code I can't \r\nsee any reason why it would be seen as a rip-off of their code unless \r\nI'm banned from coding a projection into a vector space because I read \r\nsome Matlab code doing it. See lines 1171, 1172 and 1173 in ltisys.py \r\nwhich are the core of this algorithm and are not in any way an idea \r\ncoming from Mathworks folks, these formulas are clearly written in the \r\nreference publication page 1145 and 1146.\r\n\r\n- Complex poles are not supported by the original algorithm. Using the \r\nideas from the second algorithm below I managed to reach a success rate \r\nof 90% with complex poles but this is not reliable enough so the \r\nrelevant code is disabled. Matlab has a 100% success rate in their 2013 \r\nimplementation but I don't know how they achieved that.\r\n\r\nRegarding Yang Tits algorithm:\r\n\r\n- This is an update of the KNV0 algorithm published in the mid 90's, as \r\nfar as I know it is only distributed by Slicot as \"contributed MATLAB \r\nfunctions\" http://slicot.org/contributed-software under a non-free license.\r\n\r\n- This implementation is 100% genuine, I did not read the code \r\ndistributed by slicot past the license header until my code was working \r\nso there are no licensing issues imho. After achieving a working \r\nimplementation of this algorithm I checked the reference implementation \r\nfrom Slicot and I'm still puzzled by how they did it. I'm sure there are \r\ntons of good reasons why their implementation is faster/better than mine \r\nbut I simply can't understand what they did.\r\n\r\n- Real poles are supported, sometimes the solution has a better \r\nconditionning than KNV0's, sometimes not, but it does converge must \r\nfaster all the time. I was considering adding an option to return the \r\nsolution with the best conditionning from the two methods but it might \r\nconfuse users as it would only work on real poles. What do you think ?\r\n\r\n- Complex poles are supported, I ran it for hours with random matrices \r\nand poles and it never failed.\r\n\r\nPlease let me know if this code match you quality standards and I would \r\nreally be glad if someone could test it and report any bug. The \r\nreference matlab implementations of these algorithms have been used and \r\npolished for decades, I'm sure I've missed lots of specific cases.\r\n\r\nAs a sidenote, if PR #4249 is accepted it should be possible to greatly \r\nimprove the speed of these two algorithms."},"comment":{"url":"https://api.github.com/repos/scipy/scipy/issues/comments/72886795","html_url":"https://github.com/scipy/scipy/pull/4295#issuecomment-72886795","issue_url":"https://api.github.com/repos/scipy/scipy/issues/4295","id":72886795,"user":{"login":"Eric89GXL","id":2365790,"avatar_url":"https://avatars.githubusercontent.com/u/2365790?v=3","gravatar_id":"","url":"https://api.github.com/users/Eric89GXL","html_url":"https://github.com/Eric89GXL","followers_url":"https://api.github.com/users/Eric89GXL/followers","following_url":"https://api.github.com/users/Eric89GXL/following{/other_user}","gists_url":"https://api.github.com/users/Eric89GXL/gists{/gist_id}","starred_url":"https://api.github.com/users/Eric89GXL/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Eric89GXL/subscriptions","organizations_url":"https://api.github.com/users/Eric89GXL/orgs","repos_url":"https://api.github.com/users/Eric89GXL/repos","events_url":"https://api.github.com/users/Eric89GXL/events{/privacy}","received_events_url":"https://api.github.com/users/Eric89GXL/received_events","type":"User","site_admin":false},"created_at":"2015-02-04T16:29:46Z","updated_at":"2015-02-04T16:29:46Z","body":"Have you checked code coverage? There are a lot of branches in the code, and I wonder if they are all caught by the set of tests you have (which look a bit small by comparison). It also looks like you need a rebase (sorry)."}}