Coverage increased (+0.09%) to 78.619% when pulling186d6d8 on rabraker:update_place intodf4ee82 on python-control:master.

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 viaslycot, 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 analgorithm 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 haveplace() be a wrapper that looks for the algorithm keyword and decides whether to callplace_scipy orplace_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).

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 thatscipy.signal.place_poles has a keyword argumentmethod, which can take valuesYT orKNV0. So it might make sense to have at least the three options (varga,YT,KNV0). Might be cleanest to handle the keyword argumentvarga ourselves, and just pass along the other keyword arguments toscipy.signal.place_poles, in case other algorithms are implemented inscipy.signal in the future.

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 passD 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 haveacker 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 intoplace proper. The rest can reside inplace_varga orplace_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 toplace_varga(). Also, expose thealpha parameter andC orD flag to the user. There is a bug in how thealpha 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 inplace proper. It is my understanding that this algorithm most closely resembles the matlab implementation and would be least likely to surprise users.

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.

Creating separate functions for now seems OK.

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

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

will do

Coverage decreased (-0.2%) to 78.289% when pulling311e9e2 on rabraker:update_place intodf4ee82 on python-control:master.

As requested, I have submitted thebug report and have put the oldplace function intoplace_varga.

Coverage decreased (-0.2%) to 78.289% when pulling100eb1f on rabraker:update_place intodf4ee82 on python-control:master.

Coverage decreased (-0.5%) to 77.993% when pulling45fdea9 on rabraker:update_place intodf4ee82 on python-control:master.

Coverage increased (+0.09%) to 78.618% when pulling70ecf75 on rabraker:update_place intodf4ee82 on python-control:master.

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.