Update place to use scipy.signal.place_poles #176

rabraker · 2018-01-03T07:59:34Z

This PR addresses issue #117 by using the place_poles function in scipy.signals.

Aside from just being run in matlab_test.py, there wasn't, as far as I could tell, a unit test for place previously, so I added one.

The one limitation of the algorithm used in scipy.signals is that it can't place multiple poles at the same location for SISO systems, which made the "test" in matlab_tests.place fail, so I modified the requested pole location there.

Also worth pointing that the previous implementation would fail for discrete time systems because the slycot function needed to be called with a different flag and the alpha parameter needed to be computed differently.

…gnals.place_poles

…. Also, aadd tests for control dimension checks.

coveralls · 2018-01-03T08:10:59Z

Coverage increased (+0.09%) to 78.619% when pulling 186d6d8 on rabraker:update_place into df4ee82 on python-control:master.

murrayrm · 2018-01-03T16:19:31Z

I like this change, but it brings up an interesting issue: what should we do when there are multiple algorithms available to solve a certain problem, each with different limitations? The previous version of the place command use a method by Varga (IEEE TAC, 1981), implemented via slycot, that did not have the limitation on repeated poles, but may give solutions that are less well-conditioned (as pointed out in issue #117).

As an alternative, what if we implement an algorithm keyword that allows different algorithms to be used. This would give:

place(sys, poles, algorithm='scipy'): proposed implementation in this PR
place(sys, poles, algorithm='varga'): original solution

We could have place() be a wrapper that looks for the algorithm keyword and decides whether to call place_scipy or place_varga.

Would be interested in getting thoughts on this from anyone paying attention, especially @rabraker, @vmatos, and @robertobucher (who have been involved in the conversation and code up to now).

cwrowley · 2018-01-03T16:29:19Z

I like the suggestion to add a keyword argument to specify the algorithm. I also think it would be good to retain the repeated-pole test, somehow marking it as "expected to fail" if the YT algorithm is used. Also, note that scipy.signal.place_poles has a keyword argument method, which can take values YT or KNV0. So it might make sense to have at least the three options (varga, YT, KNV0). Might be cleanest to handle the keyword argument varga ourselves, and just pass along the other keyword arguments to scipy.signal.place_poles, in case other algorithms are implemented in scipy.signal in the future.

rabraker · 2018-01-03T17:43:46Z

I think trying to cram too much functionality into a single function makes the code more difficult to test, maintain, use and document. Part of the problem is that varga and YT require different parameters. For example, to place discrete poles using varga, you need to pass D into the slycot function. This is not required with YT. Because we are passing in just matrices (A, B) and not the full LTI system, then this flag must necessarily be an argument in the wrapper function. Now, you have a parameter that only gets used sometimes and this must be explained in the documentation.

We already have acker as its own function. If it were up to me, I would keep the convention that different algorithms stay in their own functions. It can be up to us to decide which is the "canonical" implementation and gets put into place proper. The rest can reside in place_varga or place_yt or similar and will be discoverable via 'see also:' in the doc string.

My proposal would be the following:

keep the current code for place, but change the function name to place_varga(). Also, expose the alpha parameter and C or D flag to the user. There is a bug in how the alpha parameter is currently calculated that causes the placement to fail in some situations. This should probably be fixed in a separate pull request.
Keep the YT implementation in place proper. It is my understanding that this algorithm most closely resembles the matlab implementation and would be least likely to surprise users.

ilayn · 2018-01-03T18:05:07Z

Note that, the arbitrary eigenstructure assignment problem has gained a lot of attention lately and people like R. Schmid et al. seem to have the general solution to pole placement. I have asked Amit Pandey from UCSD and he kindly shared his matlab code with me. I will try to propose a PR to SciPy whenever I can find time. This means, if accepted, there will be at least one additional SciPy method.

murrayrm · 2018-01-03T18:30:58Z

Creating separate functions for now seems OK.

@rabraker Can you implement your proposal above by updating this PR to pull out the previous functionality as place_varga() and create an issue to capture the alpha parameter bug?

@ilayn If you (or someone) wants to convert the MATLAB code for the R. Schmid et al method into a separate place_XYZ method, we could include here while waiting for the official SciPy update.

rabraker · 2018-01-03T19:00:56Z

will do

…y implementation of YT stays in place() proper

coveralls · 2018-01-03T20:00:59Z

Coverage decreased (-0.2%) to 78.289% when pulling 311e9e2 on rabraker:update_place into df4ee82 on python-control:master.

rabraker · 2018-01-03T20:44:56Z

As requested, I have submitted the bug report and have put the old place function into place_varga.

coveralls · 2018-01-03T23:17:36Z

Coverage decreased (-0.2%) to 78.289% when pulling 100eb1f on rabraker:update_place into df4ee82 on python-control:master.

murrayrm · 2018-01-04T05:17:33Z

control/tests/matlab_test.py

    def testPlace(self):
-        place(self.siso_ss1.A, self.siso_ss1.B, [-2, -2])


I think we should leave this test in place, but calling place_varga (assuming slycot is installed). Also, can we add a check for the right type of error if we call place with repeated poles?

I have made this change. I have also added two place-holder tests (or whatever they are called) into matlab_test.py for place and acker.

I added an assert_raises(ValueError....) test for place() to statefbk_test.py.

murrayrm · 2018-01-04T05:18:14Z

control/tests/statefbk_test.py

+                      [0,      0,],
+                      [-3.146,  0]])
+        P = np.array([-0.5+1j, -0.5-1j, -5.0566, -8.6659])
+        K = place(A, B, P)


Can we also add some tests for place_varga and acker here, as a way to increase coverage?

I think acker already has a test (line 125)...

I added a test for place_varga.

…or repeated poles in place. Added placeholder tests in matlab_test for place and acker and reverted the repeated pole test for place varga.

coveralls · 2018-01-04T16:31:52Z

Coverage decreased (-0.5%) to 77.993% when pulling 45fdea9 on rabraker:update_place into df4ee82 on python-control:master.

coveralls · 2018-01-04T16:46:16Z

Coverage increased (+0.09%) to 78.618% when pulling 70ecf75 on rabraker:update_place into df4ee82 on python-control:master.

ilayn · 2018-01-04T21:58:09Z

If you (or someone) wants to convert the MATLAB code for the R. Schmid et al method into a separate place_XYZ method, we could include here while waiting for the official SciPy update.

I will get back to this asap and let you know.

rabraker added 2 commits January 2, 2018 17:17

Modify statefbk.place to use the YT algorithm implemented in scipy.si…

99302cd

…gnals.place_poles

YT algo cannot place multiple poles at the same location when rk(B)=1…

186d6d8

…. Also, aadd tests for control dimension checks.

Keep the original place code in a new function, place_varga. The scip…

311e9e2

…y implementation of YT stays in place() proper

rabraker mentioned this pull request Jan 3, 2018

place function fails for simple case #177

Closed

Added 'see also' to doc-strings

100eb1f

murrayrm reviewed Jan 4, 2018

View reviewed changes

rabraker added 2 commits January 4, 2018 09:15

Added tests for acker, and place_varga. Added an assert_raises test f…

45fdea9

…or repeated poles in place. Added placeholder tests in matlab_test for place and acker and reverted the repeated pole test for place varga.

Forgot to check that slycot is installed in testPlace_varga()

70ecf75

murrayrm merged commit 7c5f28d into python-control:master Jan 5, 2018

rabraker deleted the update_place branch January 5, 2018 08:39

murrayrm mentioned this pull request Dec 23, 2018

Figure out a way to specify which algorithm to run when multiple choices are available #255

Open

pierre-haessig mentioned this pull request May 15, 2024

add place_varga in API doc #1004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update place to use scipy.signal.place_poles #176

Update place to use scipy.signal.place_poles #176

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

murrayrm commented Jan 3, 2018 •

edited

Loading

cwrowley commented Jan 3, 2018

rabraker commented Jan 3, 2018

ilayn commented Jan 3, 2018

murrayrm commented Jan 3, 2018 •

edited

Loading

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

murrayrm Jan 4, 2018

rabraker Jan 4, 2018

murrayrm Jan 4, 2018

rabraker Jan 4, 2018

coveralls commented Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

ilayn commented Jan 4, 2018

		def testPlace(self):
		place(self.siso_ss1.A, self.siso_ss1.B, [-2, -2])

Update place to use scipy.signal.place_poles #176

Update place to use scipy.signal.place_poles #176

Conversation

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 • edited Loading

murrayrm commented Jan 3, 2018 • edited Loading

cwrowley commented Jan 3, 2018

rabraker commented Jan 3, 2018

ilayn commented Jan 3, 2018

murrayrm commented Jan 3, 2018 • edited Loading

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 • edited Loading

rabraker commented Jan 3, 2018

coveralls commented Jan 3, 2018 • edited Loading

murrayrm Jan 4, 2018

Choose a reason for hiding this comment

rabraker Jan 4, 2018

Choose a reason for hiding this comment

murrayrm Jan 4, 2018

Choose a reason for hiding this comment

rabraker Jan 4, 2018

Choose a reason for hiding this comment

coveralls commented Jan 4, 2018 • edited Loading

coveralls commented Jan 4, 2018 • edited Loading

ilayn commented Jan 4, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

murrayrm commented Jan 3, 2018 •

edited

Loading

murrayrm commented Jan 3, 2018 •

edited

Loading

coveralls commented Jan 3, 2018 •

edited

Loading

coveralls commented Jan 3, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading