- Notifications
You must be signed in to change notification settings - Fork441
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedAug 18, 2020
coveralls commentedAug 18, 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.
ilayn commentedAug 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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bnavigator commentedAug 18, 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.
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 git reset --hard origin/better-tfinal If you don't like my interference, to revert my rebase, just |
removed unicode in code to make python 2 happyCo-authored-by: Ben Greiner <code@bnavigator.de>
@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
|
|
I'll give up and do that if this last commit fails too. |
ilayn commentedAug 19, 2020
phew... 😅 |
@ilayn Related: your code uses is_step only for computing cont-timeresponses, but not discrete-time. Is that intentional? S On Wed, Aug 19, 2020 at 2:32 PM Ilhan Polat ***@***.***> wrote: I double checked and new code works fine with z-plane poles at zero or 1, phew... 😅 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSNLVA42RHGYN4ZT4QTSBRABVANCNFSM4QC4FPIA> . -- Sawyer FullerAssistant Professor of Mechanical EngineeringAdjunct, Paul G Allen School of Computer ScienceUniversity of Washingtonhttp://faculty.washington.edu/~minster/(Typed with my thumbs) |
ilayn commentedAug 22, 2020
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. |
@ilayn thanks, makes sense. Update on the above code: whoops, I didn't actually put a pole at the origin. updated code still works:
|
Uh oh!
There was an error while loading.Please reload this page.
Spelling error in docstringCo-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 |
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 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.
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
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
@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.