Skip to content

support discrete system as input for FRD #568

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 1 commit into from
Mar 12, 2021

Conversation

bnavigator
Copy link
Contributor

Triggered by #566 and #523, it was revealed that the evaluation of the sys parameter does not take into account discrete time systems.

@bnavigator
Copy link
Contributor Author

... needs unit tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 88.821% when pulling fd80fc8 on bnavigator:discretesys2frd into 92de12d on python-control:master.

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 to me! One remark: FRD.__call__ only works with purely imaginary arguments of the form omega*1j, which I think is still what we want, right? The alternative is if an FRD system derived from a discrete time system that z must be on the unit circle. I think I favor the former but I am not positive that is the "right" way or not.

@sawyerbfuller sawyerbfuller merged commit 0f951e1 into python-control:master Mar 12, 2021
sawyerbfuller added a commit to sawyerbfuller/python-control that referenced this pull request Mar 12, 2021
@bnavigator
Copy link
Contributor Author

I also favor the current. Otherwise you would have to keep track from which kind you created the FRD system.

@murrayrm murrayrm added this to the 0.9.0 milestone Mar 20, 2021
@bnavigator bnavigator deleted the discretesys2frd branch February 18, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants