GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Update docstring for impulse for discrete sys by sawyerbfuller · Pull Request #812 · python-control/python-control

Conversation

@sawyerbfuller

@sawyerbfuller sawyerbfuller commented Dec 10, 2022

Copy link
Copy Markdown
Contributor

Update impulse_response doc string to indicate that the impulse size is unit area for discrete systems.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sawyerbfuller
Update timeresp.py
af7a7d7
Update impulse_response doc string to indicate that the impulse size is unit area for discrete systems.
@sawyerbfuller sawyerbfuller linked an issue Dec 10, 2022 that may be closed by this pull request
Scaling difference in impulse response size compared to scipy.signal #813
Closed
@murrayrm

murrayrm commented Dec 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

We definitely need to update the documentation, but I wonder whether we shouldn't also consider what the right definition of the impulse response is for a discrete time system with non-unit sampling time. MATLAB apparently defines the impulse response for a discrete time system as being the response to an input that is 1 at the initial time and then 0 after that. This matches the description in the Wikipedia entry for impulse response.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sawyerbfuller

sawyerbfuller commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

I made this change a little while back to match what matlab does, which is a unit-area impulse for discrete-time systems: https://www.mathworks.com/help/control/ref/lti.impulse.html. This arrangement is probably what you want if you are viewing a discrete-time system as a discretized model of a continuous-time system.

Probably unit area is favored by engineering, unit magnitude by math.

Probably a subjective choice. Anybody here have further insight into such a choice?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ilayn

ilayn commented Dec 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

As the sampling frequency period grows the response starts to get larger if impulse is kept at unity. Hence the same dynamical system sampled at different frequencies will give different results with the same unity impulse which is not correct. In a hand-waving argument we have to scale the energy of the impulse with frequency to have equivalent supply. Otherwise, we need to type math here which is not fun :)

You can test it yourself by generating a [1, 0, 0, 0, ...] array and supply to lsim for different sampled versions of the same system to see the effect I am attempting to describe above.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@murrayrm

murrayrm commented Dec 13, 2022

Copy link
Copy Markdown
Member

@ilayn's comment makes it clear why scaling by the sampling time is the right approach (and I was mistaken about what MATLAB does).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@murrayrm
murrayrm merged commit 900d56e into main Dec 13, 2022
@murrayrm murrayrm added this to the 0.9.3 milestone Dec 24, 2022
@murrayrm
murrayrm deleted the sawyerbfuller-patch-2 branch June 10, 2023 05:29
@murrayrm murrayrm mentioned this pull request Mar 2, 2024
Scaling for control.impulse_response discrete time is not correct #974
Closed
@KybernetikJo KybernetikJo mentioned this pull request Jul 14, 2024
Implement ERA, change api to TimeResponseData #1024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.3

Development

Successfully merging this pull request may close these issues.

Scaling difference in impulse response size compared to scipy.signal

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@sawyerbfuller @murrayrm @ilayn
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.