-
Notifications
You must be signed in to change notification settings - Fork 442
Refactor place method #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
control/statefbk.py
Outdated
@@ -45,11 +46,11 @@ | |||
from . import statesp | |||
from .exception import ControlSlycot, ControlArgument, ControlDimension | |||
|
|||
__all__ = ['ctrb', 'obsv', 'gram', 'place', 'place_varga', 'lqr', 'acker'] | |||
__all__ = ['ctrb', 'obsv', 'gram', 'place', 'lqr'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we "demoting" place_varga
and acker
to internal functions? I know they are still accessible via the methods
keyword in place
, but seems like we should still allow direct calls to acker
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I understand we should not change the API for compatibility reasons.
Still I find it a bit confusing to have multiple access points for pole placement.
I fixed it.
control/statefbk.py
Outdated
False for continuous time pole placement or True for discrete time. | ||
The default is dtime=False. | ||
alpha: double scalar (useful only if method=="varga") | ||
If DICO='C', then _place_varga will leave the eigenvalues with real |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DICO
is not a listed argument for this function. Should this be dtime
instead? This issue pops up in other place below as well (not part of the changes, but should be fixed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it in the sphinx comment description 👍
control/statefbk.py
Outdated
try: | ||
return _place_varga(A, B, p, dtime=dtime, alpha=alpha) | ||
except ControlSlycot as error: | ||
print("[Pole placement] Fallback strategy: using Tits-Yang method from scipy.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a print
here (which can't be turned off) or should we use the warnings
package instead and call warn()
(which users can then turn off if they want)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 👍
control/statefbk.py
Outdated
try: | ||
result = place_poles(A_mat, B_mat, placed_eigs, method=method) | ||
except ValueError as error: | ||
print("[Pole placement] Redundant pole location. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
control/statefbk.py
Outdated
|
||
if alpha is None: | ||
# SB01BD ignores eigenvalues with real part less than alpha | ||
# (if DICO='C') or with modulus less than alpha | ||
# (if DICO = 'C') or with modulus less than alpha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: DICO
should be dico
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 💯
control/statefbk.py
Outdated
"""Pole placement using Ackermann method | ||
|
||
Contributed by Roberto Bucher <roberto.bucher@supsi.ch> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should include the contributor in the docstring for the function. This is internal information that is just there to give some credit for the development team, but doesn't need to be exposed to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. 👍
control/statefbk.py
Outdated
|
||
def gram(sys,type): | ||
def gram(sys, desired_computation_str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very cumbersome as can named parameter. Was there a reason to switch from type
? If so, can we find something short (ideally one word)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of a variable named as a build-in function:
https:/
8000
/docs.python.org/3/library/functions.html#type
It may break following python script:
a = 1
print(type(a))
print(type)
type = 1
print(type)
print(type(a))
leads to:
Traceback (most recent call last):
File "./test.py", line 19, in <module>
print(type(a))
TypeError: 'int' object is not callable
I suggest gramian_type
, what do you think about it?
@@ -1,15 +1,16 @@ | |||
#!/usr/bin/env python | |||
# | |||
# statefbk_test.py - test state feedback functions | |||
# test_statefbk.py - test state feedback functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we are changing the name of this file. All other files in this directory have _test
as a suffix on the file name. Suggest we leave unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw in issue #95 that you plan to organise the unit tests as in numpy. In fact, in this library as well as in many others, tests are always prefixed with test_
:
in this PR, as I refactored code related to state feedback, I also started to rename unit tests properly. Do you want me to make this change in a dedicated PR, for all other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current practice in python-control is to (mainly) use _test
at the end of the filename, so prefer to leave this as is (no need to change).
doc/control.rst
Outdated
@@ -108,7 +108,6 @@ Control system synthesis | |||
.. autosummary:: | |||
:toctree: generated/ | |||
|
|||
acker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, suggest we leave acker
as a publicly visible function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this change :)
As I said above, I think it is a little bit misleading to a several entry points for a single functionality. It increases the redundancy.
This PR has several changes, some of which seem fine but other that I have questions about. See review comments. |
91919ff
to
e3d8690
Compare
- Fix pep8 warnings - Clean some comments - Add module docstring
- Make place_varga protected -> _place_varga - Make acker protected -> _acker - place: Add optional arguments (alpha, dtime, method) - Add fallback strategy in case scipy or slycot are not found - Update docstrings - Update tests and remove pass condition in case which "slycot is not installed." - testPlace do not expect anymore a ValueError in case redundant place location is found. Instead, the algorithm fall back on the Ackermann method while printing a warning. TODO: Complete pole placement example when PR#267 is merged.
e3d8690
to
12fc6be
Compare
class ControlSlycot(Exception): | ||
"""Exception for Slycot import. Used when we can't import a function | ||
from the slycot package""" | ||
pass | ||
def __init__(self, error): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is the right approach to immediately print this error reason. This creates an exception with a by-product.
This also does not call Exception.init, (which would normally store the error string) so if caught and printed later, the original exception reason has disappeared.
This may just be personal preference, but I don't particularly like the idea of placing the poles with a method other than what the user specified (i.e., the fallback strategies). In my view, there are several drawbacks to this:
In short, if the only way to get the poles placed is by calling One final concern is that, as it stands, the docstring is not really accurate any more, because it describes (in the "algorithms" and "limitations" sections) the YT algorithm, not Ackermann's method or Varga's method or KNV0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more things to fix in the code when you make revisions.
@@ -61,6 +63,18 @@ def place(A, B, p): | |||
Input matrix | |||
p : 1-d list | |||
Desired eigenvalue locations | |||
method : string - optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Numpydoc suggests the format "string, optional" to indicate optional arguments and also suggests that the default value be specified in the description of the parameter.
return place_varga(A, B, p, dtime=dtime, alpha=alpha) | ||
except ControlSlycot as error: | ||
warnings.warn("[Pole placement] Fallback strategy: using Tits-Yang method from scipy.") | ||
return place(A, B, p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is kept, I suggest putting the method explicitly in the call to place
. Otherwise if the default is changed, the warning will not be accurate.
K = place_varga(A, B, p, dtime=dtime, alpha=alpha) | ||
return K | ||
except ControlSlycot as error: | ||
warnings.warn("[Pole placement] Fallback strategy: using Ackermanm method.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ackermann misspelled.
elif dtime and alpha < 0.0: | ||
raise ValueError("Need alpha > 0 when DICO='D'") | ||
|
||
raise ValueError("Need alpha > 0 when dico='D'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is confusing since dico
is not the name of the parameter that is passed to the function (it is dtime
).
@@ -1,15 +1,16 @@ | |||
#!/usr/bin/env python | |||
# | |||
# statefbk_test.py - test state feedback functions | |||
# test_statefbk.py - test state feedback functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current practice in python-control is to (mainly) use _test
at the end of the filename, so prefer to leave this as is (no need to change).
@don4get There are some good changes in this PR, but needs to address the comment in this this thread. Specifically:
|
PR has been idle for > 6 months => stale. @don4get I will close this PR in a few days unless you will have time to work on it soon. No worries if not; just re-open when you have responded to the comments above. |
This PR aims to clean the API concerning pole placement, and add fallback strategy in case scipy or slycot are not installed.
There is also a little bit of style refactor and unit test of
test_statefbk.py
do not require np.matrix anymore, as it will soon be deprecated.This PR is linked to this issue #255