Improved default time vector and handling for time response functions step, impulse, and initial #420
Improved default time vector and handling for time response functions step, impulse, and initial #420murrayrm merged 11 commits intopython-control:masterfrom sawyerbfuller:fix-discrete-step-response
Conversation
…ons step, impulse, and initial
|
Looks like this broke some unit tests? |
…d number of steps rather than complete time vector in timeresponse functions
sawyerbfuller
changed the title
…umpy instead of scipy
…umpy instead of scipy
|
Ok take a look, fixed the tests. Also implemented a convenience that you can now specify T as a scalar in step, impulse, and initial to quickly change the length of the simulation. Easier than specifying the whole T vector every time (which you can still do). This is similar to how you can do it in bode, and how Matlab does it. I think these conveniences will not break compatibility with extant code. |
|
I’m not sure how solidified things are considered to be in the package, so I’m making guesses, trying balance adding functionality I would like to have, and avoiding breaking backwards compatibility. Please let me know if I am going too far in one direction or the other! If these changes seem ok, I can add a couple more unit tests. |
|
Looks like this PR addresses half of the issue of issue #287 , but I essentially just used the same coarse algorithm to estimate a reasonable time horizon that scipy uses. And expanded it to work with Discrete-time systems. |
|
This looks good to me. I need to check it a bit more carefully, but nice to have a proper method for computing response times and some enhancements on how to specify the final time. |
…n system poles. and unit tests.
|
Great. I took a stab at solving the other half of #287, automatically generating a default dt from the fastest system eigenvalues as well, if the time vector is not specified.
|
sawyerbfuller
changed the title
|
I hope that for now this is maybe a “good enough” version of the more comprehensive solution that ilayn was working on in #287 – that also adds discrete-time support. |
murrayrm
left a comment
murrayrm
left a comment
There was a problem hiding this comment.
Changes look fine in general. A couple of questions regarding updates to unit tests (to make sure I understand) and some questions about documentation. No changes required for approval unless you feel they are justified.
control/tests/sisotool_test.py
Outdated
|
|
||
| # Check the step response before moving the point | ||
| step_response_original = np.array([ 0., 0.02233651, 0.13118374, 0.33078542, 0.5907113, 0.87041549, 1.13038536, 1.33851053, 1.47374666, 1.52757114]) | ||
| step_response_original = np.array([0. , 0.0217, 0.1281, 0.3237, 0.5797, 0.8566, 1.116 , 1.3261, 1.4659, 1.526 ]) |
There was a problem hiding this comment.
Why do these values need to be changed? The system is the same and so shouldn't the step response be the same?
There was a problem hiding this comment.
The function in scipy that used to be used to generate the time vector, scipy.signal.ltisys._default_response_times(A, n) uses numpy.linspace without the keyword arg endpoint=False so it returns n+1 data points instead of n. The new function does, so it generates only n data points, which is consistent with how other functions in python-control seem to do it. (thus the slightly changed values). Let me know if you have a preference to go back to n+1 datapoints. My thinking is, if you ask for n datapoints, that ought to be how many you get.
control/tests/sisotool_test.py
Outdated
|
|
||
| # Check if the step response has changed | ||
| step_response_moved = np.array([[ 0., 0.02458187, 0.16529784 , 0.46602716 , 0.91012035 , 1.43364313, 1.93996334 , 2.3190105 , 2.47041552 , 2.32724853] ]) | ||
| step_response_moved = np.array([0. , 0.0239, 0.161 , 0.4547, 0.8903, 1.407 , 1.9121, 2.2989, 2.4686, 2.353 ]) |
There was a problem hiding this comment.
Same question as above. Just want to make sure I understand the need for the change.
control/timeresp.py
Outdated
| Time vector (argument is autocomputed if not given) | ||
| T: array-like or number, optional | ||
| Time vector, or simulation time duration if a number (time vector is | ||
| autocomputed if not given) |
There was a problem hiding this comment.
Should we document someplace (perhaps in doc/) how this is autocomputed?
There was a problem hiding this comment.
I couldn't find an obvious place in doc about where to put it so I put in the docstring of step_response and referred back to that docstring in impuse, initial, and step_info.
|
|
||
| T_num: number, optional | ||
| Number of time steps to use in simulation if T is not provided as an | ||
| array (autocomputed if not given); ignored if sys is discrete-time. |
There was a problem hiding this comment.
Document someplace how these are autocomputed?
3 participants
step, impulse, and initial, and step_info, as well as their matlab equivalents
_get_default_tfinal(sys)_get_ideal_tfinal_and_dt(sys)that computes a time window equal to seven times the slowest time constant of sys, inspired by scipy.signal.ltisys._default_response_times(A, n), that can also accommodate discrete-time systems