8000 Refactor place method by don4get · Pull Request #268 · python-control/python-control · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 7 commits into from

Conversation

don4get
Copy link
Contributor
@don4get don4get commented Jan 5, 2019

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

@coveralls
Copy link
coveralls commented Jan 5, 2019

Coverage Status

Coverage decreased (-0.5%) to 77.814% when pulling 12fc6be on don4get:refactor/statefbk into 1254ccb on python-control:master.

@don4get don4get changed the title Refactor statefbk.py Issue https://github.com/python-control/python-control/issues/255 Refactor place method Jan 5, 2019
@don4get don4get changed the title Issue https://github.com/python-control/python-control/issues/255 Refactor place method Refactor place method Jan 5, 2019
@don4get don4get mentioned this pull request Jan 8, 2019
@@ -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']
Copy link
Member

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?

Copy link
Contributor Author

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.

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
Copy link
Member

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).

Copy link
Contributor Author

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 👍

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.")
Copy link
Member

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

try:
result = place_poles(A_mat, B_mat, placed_eigs, method=method)
except ValueError as error:
print("[Pole placement] Redundant pole location. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warn()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 💯

"""Pole placement using Ackermann method

Contributed by Roberto Bucher <roberto.bucher@supsi.ch>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. 👍


def gram(sys,type):
def gram(sys, desired_computation_str):
Copy link
Member

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)?

Copy link
Contributor Author
@don4get don4get Apr 13, 2019

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
Copy link
Member

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.

Copy link
Contributor Author

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_:
image

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?

Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor Author
@don4get don4get Apr 13, 2019

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.

@murrayrm
Copy link
Member
murrayrm commented Apr 6, 2019

This PR has several changes, some of which seem fine but other that I have questions about. See review comments.

don4get added 7 commits April 13, 2019 16:18
- 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.
@don4get don4get force-pushed the refactor/statefbk branch from e3d8690 to 12fc6be Compare April 13, 2019 14:18
class ControlSlycot(Exception):
"""Exception for Slycot import. Used when we can't import a function
from the slycot package"""
pass
def __init__(self, error):
Copy link
Member

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.

@rabraker
Copy link
Contributor

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:

  1. The code to achieve this is rather complex. This makes it more difficult to reason about what the function will actually do and also makes testing all the possible code paths harder. For instance, the path "varga" -> "YT" -> "acker" is, as far as I can tell, not exercised in the updated test suite.

  2. The second problem is that the methods are not really equivalent. For example, if the user specified "varga", and that fails for some reason, now we call place with "YT". But unless alpha is None, the result will not be what the user wanted. Similarly for devolving to acker when "YT" fails, because "YT" is numerically stable but acker is not. This can matter a great deal for some systems.

  3. It is possible that this PR will break users code. It is not too far fetched to imagine a scenario (e.g., a parameter sweep) that relies on place producing an error when two desired pole locations become too close. This PR would break (or at least change the behavior of) such a script.

In short, if the only way to get the poles placed is by calling place with "acker" or whatever, I don't see the downside to just letting the user decide that.

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.

Copy link
Member
@murrayrm murrayrm left a 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
Copy link
Member

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)
Copy link
Member

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.")
Copy link
Member

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'.")
Copy link
Member

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
Copy link
Member

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).

@murrayrm
Copy link
Member

@don4get There are some good changes in this PR, but needs to address the comment in this this thread. Specifically:

  • We need to rethink the "automatic" use of a backup strategy, as pointed out by @rabraker. One possibility would be to allow method to be a list, where you try the next method in the list if the first fails. Another would be to add a keyword fallback that would default to False (to preserve existing behavior) but that could be used to enable the use of fallback strategies.

  • @repagh's comments on the the ControlSlycot exception should be addressed. Note also that PR More unit tests #305 makes the ControlSlycot exception derive from ImportError instead of Exception.

  • Please revert test_statefbk.py back tot statefbk_test.py to be consistent with the predominant naming convention for test files in tests/ (I understand this is different than numpy),

  • Other small comments raised in the reviews should be resolved.

  • Need to resolve conflicts against the current master branch.

@murrayrm murrayrm added the onhold Waiting for changes or input label May 27, 2019
@murrayrm
Copy link
Member

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.

@murrayrm murrayrm closed this Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
onhold Waiting for changes or input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0