Fix place varga#197
Conversation
murrayrm
left a comment
There was a problem hiding this comment.
A couple of suggestions for your consideration.
| # SB01BD ignores eigenvalues with real part less than alpha | ||
| # (if DICO='C') or with modulus less than alpha | ||
| # (if DICO = 'D'). | ||
| if DICO == 'C': |
There was a problem hiding this comment.
What happens if the users passes something other than 'C' or 'D' for DICO? There should probably be a final else that generates an error message. Also, should we allow 'c' and 'd' (lower case) as well?
Alternatively, use isdtime as the option and then there are only two possibilities (True or not True).
|
|
||
| Optional Parameters | ||
| --------------- | ||
| DICO : 'C' for continuous time pole placement or 'D' for discrete time. |
There was a problem hiding this comment.
The use of DICO as a parameter seems odd since it is not used in other functions. A couple of possibilities:
_rss_generate(internal function) usestypegramusesdico(lower case) as an internal variable- We use use
isdtimeandisctimeas functions to check for discrete and continuous time. We could useisdtime=Falseas the option.
There was a problem hiding this comment.
I am also in favour of isdtime, or maybe simply "dtime". The C / D character is a really Fortran-like approach.
There was a problem hiding this comment.
Agree that dtime=False would be a better choice for the option (and default value).
…ntrol#177 by changing how the alpha parameter is computed. In additional, expose alpha to the user an optional parameter
4f3dc15 to
33c59a8
Compare
|
Thanks for the updates. I think this looks good to go. I will do some local testing and then merge in the coming days. |
|
Tested locally and looks fine. Merging. |
This pull request address issue #177 .
Summary of Changes:
Calculate the alpha parameter such it is always twice as small as the smallest (real part) open-loop eigenvalue.
Add support for discrete time systems by exposing the DICO parameter
Expose the alpha parameter to the user, since it seems like interesting functionality to be able to place only the slowest eigenvalues.
Add tests to exercise these changes, including a test against the example in issue place function fails for simple case #177 .