Skip to content

use __call__ instead of evalfr in lti system classes #449

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 36 commits into from
Jan 8, 2021

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfuller sawyerbfuller commented Aug 14, 2020

fixes problem reported in #434 and extends #179.

  • move to __call__ as primary frequency response method to be used in TransferFunction, StateSpace, and FrequencyResponseData systems. (__call__ is new for StateSpace, and FrequencyResponse)
  • the new __call__ has same interface for all three classes, and can now take one or an array of s, unlike the old _evalfr, which could only take one s
  • it also adds a convenient keyword argument squeeze=True that automatically squeezes output if sys is SISO
  • for FrequencyResponseData.__call__(s), s must be purely imaginary or an error is raised
  • private method _evalfr that was inconsistent with matlab module matlab.evalfr was removed in favor of __call__
  • method sys.evalfr(s) has been deprecated for 2.5 years and is now removed. use __call__ instead, or matlab.evalfr(sys, s) (following discussion in lti.evalfr and sys._evalfr have different behavior #434)
  • method sys.freqresp(omega) is now deprecated. Use new pythonic methodsys.frequency_respose instead, or freqresp(sys, omega) from matlab module (following discussion in lti.evalfr and sys._evalfr have different behavior #434)
  • horner(s) now does the same thing for both TransferFunction and StateSpace systems: can evaluate at multiple values of s
  • de-duplication of code, cleanup, pythonization and vectorization of code where possible
  • FRD.eval behavior was left as-is, except for an optional squeeze argument

Copy link
Contributor

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

There are a couple of failing unit tests to still iron out

May I point your interest towards #438, where some of the evalfr/_evalfr/freqresp etc. test have been treated by the iron already. Especially some continuous/discrete inconsistency.

@sawyerbfuller
Copy link
Contributor Author

@bnavigator any idea how soon #438 will be ready for prime time? I can plan to try to merge (and figure out how to do that) when you’re done with the PR.

@bnavigator
Copy link
Contributor

Hi,

I think it is ready. But it depends on the yet unmerged PRs mentioned in the initial post. We should get those approved and merged first. I understand #438 is hard to review. The diff will get smaller with the underlying PRs though.

@sawyerbfuller sawyerbfuller changed the title [WIP] use __call__ instead of evalfr in lti system classes use __call__ instead of evalfr in lti system classes [done] Aug 16, 2020
@coveralls
Copy link

coveralls commented Aug 16, 2020

Coverage Status

Coverage decreased (-0.07%) to 87.438% when pulling a631098 on sawyerbfuller:call-method into 4103688 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 84.032% when pulling 2c4ac62 on sawyerbfuller:call-method into d3142ff on python-control:master.

@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Aug 16, 2020

A few comments: sys.horner was modified slightly so that it only takes care of the low-level algorithm, while __call__ is intended to perform user friendly tasks like input formatting and squeezing the output as necessary.

Copy link
Contributor

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

Please keep the numpydoc style

Copy link
Contributor

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

more numpydoc

@bnavigator bnavigator added this to the 0.9.0 milestone Aug 16, 2020
@bnavigator bnavigator linked an issue Aug 16, 2020 that may be closed by this pull request
sawyerbfuller and others added 10 commits August 17, 2020 10:49
Co-authored-by: Ben Greiner <code@bnavigator.de>
Co-authored-by: Ben Greiner <code@bnavigator.de>
doc fix

Co-authored-by: Ben Greiner <code@bnavigator.de>
suggested doctoring fix for statesspace.slycot_horner

Co-authored-by: Ben Greiner <code@bnavigator.de>
doctoring fixes to adhere to numpydoc convention

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

most recent commit moves the task of calling slycot and fall-back if it doesn't work into sys.horner

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

sawyerbfuller commented Jan 7, 2021

ok I think ive resolved doc comments with the last two commits

Travis isnt happy about somethng though.

@bnavigator
Copy link
Contributor

Build 940 and 941 got canceled and 942 didn't start yet. We have to wait a little.

And the day when we can ditch Travis for a different CI will be a happy day for me. (Working on Github Actions for Slycot right now in python-control/Slycot#140, and then I will tackle #446)

@sawyerbfuller
Copy link
Contributor Author

Ok looks like Travis is happy now. I’m happy to have it merged but don’t understand the various options, happy to leave it to @murrayrm and @bnavigator discretion.

@murrayrm murrayrm merged commit 0eb4396 into python-control:master Jan 8, 2021
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.

lti.evalfr and sys._evalfr have different behavior
5 participants