Skip to content

standardize time response return values, return_x/squeeze keyword processing #511

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 3 commits into from
Jan 19, 2021

Conversation

murrayrm
Copy link
Member

This PR is a partial replacement for #507, focused on addressing the issues in #453 relating to time responses:

  • Update forced_response to only return the state trajectory if return_x is True, consistent with all other time response functions (including input_output_response). Prior to this, forced_response always returned the state vector. In addition to the inconsistency, this was also problematic if the system you were simulating was a transfer function, since the state space depended on the realization and this information is lost. Some other smaller changes related to this:

    • Issue a warning if you ask for the state space trajectory corresponding to a transfer function
    • Set the default value of return_x using `config.defaults['forced_response.return_x'] (default = False)
    • Reset the default value of config.defaults['forced_response.return_x'] to True with use_legacy_defaults for versions < 0.9.0 for backward compatibility.
    • Unit tests to make sure all of that is working.
  • Consolidate processing of time responses through a new internal function _process_time_response that provides uniform processing of the return_x and squeeze keywords. This is mainly in preparation for any future changes in how we handle squeeze, so that changes only need to be made at one location in the code. The functionality of the code is unchanged, but the processing is a big different:

    • The default value for the squeeze keyword for time responses is now set in config.defaults['control.squeeze_time_response']. This is currently set to True, which means that time responses for systems with a single output are squeezed to a 1D response.
    • Added some unit tests to make sure that we get the right return values for all of the current time responses (impulse_response, initial_response, step_response, forced_response, input_output_response).
    • Added some commented out code for future implementation of a squeeze='siso' capability that would allow time responses to mirror current frequency response behavior (squeeze=True only removes axes for SISO systems). I left this out for now pending resolution of the frequency response squeeze processing (drafted in Standardize squeeze processing in frequency response functions #507, but needs more thought and effort).

Other changes, not directly related to those above

  • Added a warning if you try to specify a non-zero initial state when simulating a transfer function.
  • Docstring cleanup to use numpydoc syntax in few places.

@coveralls
Copy link

coveralls commented Jan 17, 2021

Coverage Status

Coverage increased (+0.06%) to 87.607% when pulling 47bbf16 on murrayrm:standardize_time_response into 6f674cf on python-control:master.

@murrayrm murrayrm force-pushed the standardize_time_response branch from d9acc3e to 47bbf16 Compare January 19, 2021 16:59
@murrayrm murrayrm merged commit 0a08ff2 into python-control:master Jan 19, 2021
@murrayrm murrayrm deleted the standardize_time_response branch January 19, 2021 18:06
@murrayrm murrayrm added this to the 0.9.0 milestone Mar 20, 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.

2 participants