Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
murrayrm merged 7 commits intopython-control:masterfromsawyerbfuller:better-tfinal
Oct 17, 2020

Conversation

sawyerbfuller
Copy link
Contributor

Improves the calculation of the default time vector used intimeresp 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.

rlegnain reacted with thumbs up emoji
@coveralls
Copy link

Coverage Status

Coverage decreased (-10.2%) to 74.014% when pullingd1da3e1 on sawyerbfuller:better-tfinal into7af3fbf on python-control:master.

@coveralls
Copy link

coveralls commentedAug 18, 2020
edited
Loading

Coverage Status

Coverage increased (+2.09%) to 86.303% when pullingc588cdc on sawyerbfuller:better-tfinal into5c79fc8 on python-control:master.

@ilayn
Copy link

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.

rlegnain reacted with thumbs up emoji

@bnavigator
Copy link
Contributor

bnavigator commentedAug 18, 2020
edited
Loading

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 remoteorigin:

git reset --hard origin/better-tfinal

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

sawyerbfullerand others added2 commitsAugust 19, 2020 11:11
removed unicode in code to make python 2 happyCo-authored-by: Ben Greiner <code@bnavigator.de>
@sawyerbfuller
Copy link
ContributorAuthor

@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

@bnavigatorbnavigator linked an issueAug 19, 2020 that may beclosed 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.

@bnavigatorbnavigator linked an issueAug 19, 2020 that may beclosed by this pull request
@sawyerbfuller
Copy link
ContributorAuthor

  • 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

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

phew... 😅

sawyerbfuller reacted with laugh emoji

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedAug 19, 2020 via email

@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
Copy link

@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
ContributorAuthor

@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 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

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
ContributorAuthor

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.

@bnavigatorbnavigator added this to the0.8.4 milestoneOct 17, 2020
@murrayrmmurrayrm merged commit2d7aad0 intopython-control:masterOct 17, 2020
@sawyerbfullersawyerbfuller deleted the better-tfinal branchNovember 30, 2020 19:30
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ilaynilaynilayn left review comments

@bnavigatorbnavigatorbnavigator left review comments

@namannimmo10namannimmo10namannimmo10 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.4
6 participants
@sawyerbfuller@coveralls@ilayn@bnavigator@namannimmo10@murrayrm

[8]ページ先頭

©2009-2025 Movatter.jp