Skip to content

fix time series inconsistency between continuous and discrete time simulations #295

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 2 commits into from
May 2, 2019

Conversation

murrayrm
Copy link
Member

As pointed out in issue #239, there is an inconsistency between the way that continuous time simulations and discrete time simulations return their outputs for single output systems. In continuous time, the code follows the documentation and the following works:

t, y = step_response(sys)
plot(t, y)

However, in discrete time simulations the returned value for y had shape (1, len(t)), which meant that you had to do the following for discrete time systems:

t, y = step_response(sysd)
plot(t, y[0])

The problem was that for continuous time simulations forced_response() (the ultimate function that gets called) had an np.squeeze() at the end but this wasn't present for discrete time simulations. This PR fixes that problem so that the behavior is consistent, resolving the bug in issue #239.

Other small changes:

  • Added unit tests that catch the original bug

  • Added a squeeze keyword that controls whether this behavior is implemented (default = True).

  • Did some PEP8 cleanup on timeresponse.py

Note that this may make some existing code break since discrete time simulations can now return an output that is a 1D array instead of 2D. The fix is either to remove the output index (y[0] becomes y) or set the squeeze keyword to False).

@coveralls
Copy link

coveralls commented Apr 21, 2019

Coverage Status

Coverage increased (+0.006%) to 78.228% when pulling f474179 on murrayrm:fix_time_series into d8d0a9c on python-control:master.

Copy link
Member

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


else:
# Discrete type system => use SciPy signal processing toolbox
if (sys.dt != True):
if (sys.dt is not True):
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why there are brackets around the test?

@murrayrm murrayrm merged commit c765811 into python-control:master May 2, 2019
@murrayrm murrayrm deleted the fix_time_series branch May 12, 2019 05:49
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.

3 participants