Skip to content
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

improve forced_response and its documentation #588

Merged
merged 12 commits into from Apr 2, 2021

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Mar 24, 2021

  • Fix doc error for return_x as revealed by control.forced_response argument T #586.
  • Remove redundant kwarg defaults. They are in the call signuature. Numpydoc compliant default indication
  • Enhance error message if ctime system without timevector is provided
  • Make sure the returned time vector keeps it's length

control/timeresp.py Outdated Show resolved Hide resolved
control/timeresp.py Outdated Show resolved Hide resolved
@bnavigator
Copy link
Contributor Author

bnavigator commented Mar 24, 2021

Strange that the markov test fails here too.

https://github.com/python-control/python-control/pull/588/checks?check_run_id=2188093854#step:5:212

A rerun succeeded. Wonder what triggered the length mismatch of Y and U.

@coveralls
Copy link

coveralls commented Mar 24, 2021

Coverage Status

Coverage increased (+0.2%) to 89.448% when pulling a8b72f5 on bnavigator:timeresp-doc into f1a9860 on python-control:master.

control/timeresp.py Outdated Show resolved Hide resolved
control/timeresp.py Outdated Show resolved Hide resolved
control/timeresp.py Outdated Show resolved Hide resolved
control/tests/timeresp_test.py Outdated Show resolved Hide resolved
control/timeresp.py Outdated Show resolved Hide resolved
control/timeresp.py Show resolved Hide resolved
control/timeresp.py Outdated Show resolved Hide resolved
@bnavigator bnavigator changed the title update parameter doc and error message for forced_response improve forced_response and its documentation Mar 26, 2021
@bnavigator
Copy link
Contributor Author

bnavigator commented Mar 26, 2021

Even without the modifications of this PR, testMarkovResults started to fail sporadically (#588 (comment)), because scipy.signal.dlsim returns not enough samples when the last element of the time series is just below a multiple of sys_dt

Fixed with a6ea296

TODO: cover the new lines. They were only triggered by the testMarkovResults test depending on the random seed.

@bnavigator bnavigator linked an issue Mar 30, 2021 that may be closed by this pull request
bnavigator added a commit to bnavigator/python-control that referenced this pull request Mar 31, 2021
bnavigator added a commit that referenced this pull request Mar 31, 2021
xfail testmarkovResults until #588 is merged
Copy link
Contributor

@sawyerbfuller sawyerbfuller left a comment

Choose a reason for hiding this comment

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

Looks good. Seems reasonable to allow a nonzero start time!

@bnavigator bnavigator merged commit 5434318 into python-control:master Apr 2, 2021
@murrayrm murrayrm added this to the 0.9.1 milestone Dec 30, 2021
@bnavigator bnavigator deleted the timeresp-doc branch February 18, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

control.forced_response argument T
4 participants