Skip to content

improved default time vector for time response functions that takes zeros into account. #454

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

Merged
merged 7 commits into from
Oct 17, 2020

Conversation

sawyerbfuller
Copy link
Contributor

Improves the calculation of the default time vector used in timeresp functions, e.g. step_response. Inspired by problems that appeared in #440 with pole-zero cancellations confusing the step response default time vector computation.

I combined the logic & glue from #420 with Ilayn's code from here that does a more thorough job computing default trinal and dt: https://github.com/ilayn/harold/blob/master/harold/_time_domain.py#L280-L454

  • a bit of docstring cleanup too.

@ilayn please let me know if you're not ok with having your code pasted in like this (I noted in the code that it is your work), or if licensing information needs to be added, thanks.

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.2%) to 74.014% when pulling d1da3e1 on sawyerbfuller:better-tfinal into 7af3fbf on python-control:master.

@coveralls
Copy link

coveralls commented Aug 18, 2020

Coverage Status

Coverage increased (+2.09%) to 86.303% when pulling c588cdc on sawyerbfuller:better-tfinal into 5c79fc8 on python-control:master.

@ilayn
Copy link

ilayn commented Aug 18, 2020

Thank you very much for this. It's all good. Feel free to butcher as you see fit and excuses in advance if I made mistakes here and there.

@bnavigator
Copy link
Contributor

bnavigator commented Aug 18, 2020

Sorry, the merge of #441 created a merge conflict mainly due to whitespace changes. I rebased to current master for you.

To continue working, if you don't have local changes and your personal fork is the remote origin:

git reset --hard origin/better-tfinal

If you don't like my interference, to revert my rebase, just git push -f your local branch again.

sawyerbfuller and others added 2 commits August 19, 2020 11:11
removed unicode in code to make python 2 happy

Co-authored-by: Ben Greiner <code@bnavigator.de>
@sawyerbfuller
Copy link
Contributor Author

@bnavigator all looks good. I double checked and new code works fine with z-plane poles at zero or 1, so #441 is no longer needed, except for if it adds a unit test

sys = tf(1, [1, -1], True)
plt.plot(*control.step_response(sys))
sys = tf(1, [1,], True)
plt.plot(*control.step_response(sys))

image

@bnavigator bnavigator linked an issue Aug 19, 2020 that may be closed by this pull request
@bnavigator
Copy link
Contributor

  • avoid log(0) in automatic timevector calculation #441 is already merged. Excluding many automatic PEP8 whitespace fixes from my IDE, it contained only the one line deselecting the pole. Your PR will replace that small fix with new code. Only the exact content of the lines you are replacing changed. Hence the merge conflict and required rebase.
  • The inline code comments contain a few more unicode symbols. We will need to abandon Python 2 eventually 🙄 Maybe adding an encoding header to the top is easier here.

@bnavigator bnavigator linked an issue Aug 19, 2020 that may be closed by this pull request
@sawyerbfuller
Copy link
Contributor Author

  • The inline code comments contain a few more unicode symbols. We will need to abandon Python 2 eventually 🙄 Maybe adding an encoding header to the top is easier here.

I'll give up and do that if this last commit fails too.

@ilayn
Copy link

ilayn commented Aug 19, 2020

I double checked and new code works fine with z-plane poles at zero or 1,

phew... 😅

@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Aug 19, 2020 via email

@ilayn
Copy link

ilayn commented Aug 22, 2020

@ilayn Related: your code uses is_step only for computing cont-time responses, but not discrete-time. Is that intentional? S

Yes, because in discrete time, we don't really need to have to get the fast dynamics contributions since they are bounded by the sampling period. I might be wrong though, didn't give too much of a careful thought. Probably can be tested with very fast poles and very slow poles.

@sawyerbfuller
Copy link
Contributor Author

@ilayn thanks, makes sense.

Update on the above code: whoops, I didn't actually put a pole at the origin. updated code still works:

sys = tf(1, [1, -1], True)
plt.stem(*control.step_response(sys))
sys = tf(1, [1, 0], 1)
plt.stem(*control.step_response(sys), markerfmt='C1o', linefmt='C1')

image

Spelling error in docstring

Co-authored-by: Naman Gera <namangera15@gmail.com>
@@ -65,12 +65,18 @@
capability and better automatic time vector creation
Date: June 2020

Modified by Ilhan Polat to improve automatic time vector creation
Copy link

Choose a reason for hiding this comment

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

I think you should get the credit here as far as I am concerned. I really didn't do anything other than geeking out some years ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Geeking out = valuable! : ) If you're ok with it, I would just say leave it in - I took credit elsewhere in the code for merging things together.

@bnavigator bnavigator added this to the 0.8.4 milestone Oct 17, 2020
@murrayrm murrayrm merged commit 2d7aad0 into python-control:master Oct 17, 2020
@sawyerbfuller sawyerbfuller deleted the better-tfinal branch November 30, 2020 19: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
6 participants