- Notifications
You must be signed in to change notification settings - Fork441
Improved default time vector and handling for time response functions step, impulse, and initial#420
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Improved default time vector and handling for time response functions step, impulse, and initial#420
Uh oh!
There was an error while loading.Please reload this page.
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
…umpy instead of scipy
…umpy instead of scipy
coveralls commentedJun 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
sawyerbfuller commentedJun 21, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
|
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. |
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.
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
| @@ -27,7 +27,7 @@ def test_sisotool(self): | |||
| assert_array_almost_equal(ax_rlocus.lines[2].get_data(),initial_point_2) | |||
| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The function in scipy that used to be used to generate the time vector,scipy.signal.ltisys._default_response_times(A, n) usesnumpy.linspace without the keyword argendpoint=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
| @@ -59,7 +59,7 @@ def test_sisotool(self): | |||
| assert_array_almost_equal(ax_mag.lines[0].get_data()[1][10:20],bode_mag_moved) | |||
| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we document someplace (perhaps indoc/) how this is autocomputed?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Document someplace how these are autocomputed?
Uh oh!
There was an error while loading.Please reload this page.
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