Skip to content

improve forced_response and its documentation #588

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 2, 2021
Merged

improve forced_response and its documentation #588

merged 12 commits into from
Apr 2, 2021

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Mar 24, 2021 •
edited
Loading

  • 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

sawyerbfuller
sawyerbfuller reviewed Mar 24, 2021
View reviewed changes
sawyerbfuller
sawyerbfuller reviewed Mar 24, 2021
View reviewed changes
@bnavigator
Copy link
Contributor Author

bnavigator commented Mar 24, 2021 •
edited
Loading

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 •
edited
Loading

Coverage Status

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

sawyerbfuller
sawyerbfuller reviewed Mar 25, 2021
View reviewed changes
murrayrm
murrayrm reviewed Mar 25, 2021
View reviewed changes
@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 •
edited
Loading

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 mentioned this pull request Mar 28, 2021
Merged
@bnavigator bnavigator linked an issue Mar 30, 2021 that may be closed by this pull request
control.forced_response argument T #586
Closed
@bnavigator bnavigator mentioned this pull request Mar 30, 2021
Merged
bnavigator added a commit to bnavigator/python-control that referenced this pull request Mar 31, 2021
@bnavigator
xfail testmarkovResults until python-control#588 is merged
b0179fb
bnavigator added a commit that referenced this pull request Mar 31, 2021
@bnavigator
Merge pull request #601 from bnavigator/master
9edc768 
xfail testmarkovResults until #588 is merged
sawyerbfuller
sawyerbfuller approved these changes Apr 1, 2021
View reviewed changes
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
Reviewers

@sawyerbfuller sawyerbfuller sawyerbfuller approved these changes

@murrayrm murrayrm Awaiting requested review from murrayrm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.

control.forced_response argument T
4 participants
@bnavigator @coveralls @murrayrm @sawyerbfuller