Skip to content

Time response data class #649

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Nov 21, 2021
Merged

Time response data class #649

merged 14 commits into from
Nov 21, 2021

Conversation

murrayrm
Copy link
Member

This PR adds a TimeResponseData class, following the discussion in #645, which provides a more object-oriented approach to the representation of time response data. All time responses functions now return an instance of TimeResponseData containing the results of simulations. The class implements an __iter__ method that allows backward compatibility with current call signatures.

Old approach (still works):

t, y = ct.step_response(siso_sys)
plt.plot(t, y)

t, y = ct.step_response(mimo_sys)
plt.plot(t, y[0, 1], label="input 1 to output 0")
plt.plot(t, y[1, 1], label="input 1 to output 1")

New approach:

response = ct.step_response(siso_sys)
plt.plot(response.time, response.outputs)

response = ct.step_response(mimo_sys)
plt.plot(response.time, response.outputs[0, 1], label="input 1 to output 0")
plt.plot(response.time, response.outputs[1, 1], label="input 1 to output 1")

The attributes time, outputs, states and inputs are actually class properties that carry out "squeeze processing" allowing use of 1D arrays for SISO systems. Time response data can also be accessed via the attributes t, y, x, and u, which are always in "MIMO" format (indexed by output, input (trace), and time:

response = ct.step_response(siso_sys)
plt.plot(response.t, response.y[0, 0])

Other notes:

  • I adopted the terminology of a "trace" to handle the time response data returned by step_response and impulse_response, where a MIMO system generates a full matrix of input/output responses. The output, state, and input responses for those functions are represented by a 3D array indexed by output/state/input, trace, and time.

  • The terminology for time, outputs, states and inputs was chosen to be consistent with the OptimalControlResult class. This is implemented via a set of properties that perform the squeeze (and transpose) processing to provide compatibility with our existing functionality.

  • The InputOutputData class is currently only used to store the output of simulations, but it is set up to serve as a pure data representation class (similar to FrequencyResponseData) and could later be useful for things like input to system identification routines.

  • No changes to existing unit tests were needed => should be fully backwards compatible.

At a future date, we may want to consider adding some additional functionality:

  • Plot methods that plot input/output simulations in a uniform way
  • Methods to convert data into DataFrame format for use of pandas plotting capabilities

I'm still not completely sure if this is a useful PR, but I was motivated by the way a similar approach taken in JuliaControl and thought I would give it a try. Suggestions and comments welcome (will leave in draft form for a bit to allow for discussion).

@coveralls
Copy link

coveralls commented Aug 26, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 90.079% when pulling 136d6f4 on murrayrm:timeresponse_class into 18976c1 on python-control:master.

@sawyerbfuller
Copy link
Contributor

If you call me a skeptic about object-oriented software I'll answer "guilty as charged" but this seems pretty cool and would work well with #645. I also like the potential utility for future system identification routines, and for future plotting functionality. Presumably when the latter comes we can do things like add label text, which can come in handy.

It would be nice, in the interest of minimizing friction for quick analyses of SISO systems, to have the shorter t, y attributes be the ones that are auto-squeezed, but if there is already a convention established in OptimalControlResult then maybe we should stick with that.

I'm not sure I understand the concept of a trace. is that different than the 3D array that encodes [output, input, time]?

@murrayrm
Copy link
Member Author

The latest push fixes an inconsistency in the way that squeeze processing was handled for states versus input/outputs. In the original code, squeeze processing was never applied to the state vector. The consequence was that if you used squeeze=True or squeeze=False then you got a different action on the state vector than the output vector.

To avoid breaking code, I set things up so that if you access the state vector through the old interface (by assigning one of the time response systems to a tuple with return_x=True) then you get the old behavior. But if you use the new interface (response.states) the squeeze processing matches what is done on the input/output vectors. Namely, setting squeeze=True will get rid of all 1 dimensional entries and setting squeeze=False will index everything by the state, trace (corresponding to the input number of step_response and impulse_response), and time (in that order), even for SISO systems. If you leave squeeze=None then the trace is removed for SISO systems but maintained for MIMO systems.

As in the original PR, all legacy code continues to run unchanged and there are new unit tests to validate the updated behavior for the new interface.

Alternatively, I would be OK making everything consistent across output, state, and input squeeze processing, but then we would break existing code. So my thought was to leave that in place and eventually deprecate it (perhaps after we have implemented all of the suggestions in #645).

@murrayrm
Copy link
Member Author

murrayrm commented Aug 27, 2021 •
edited
Loading

I'm not sure I understand the concept of a trace. is that different than the 3D array that encodes [output, input, time]?

It's the same. I decided to use the term "trace" because in the case of step/impulse response it happens to be the case that each trace (simulation run) corresponds to choosing one of the inputs. But more generally, you could imagine having multiple input/output traces (eg, for the aforementioned system ID) and a given trace might just be come combination of inputs.

Note also that TimeResponseData includes an inputs property of dimension (ninputs, ntraces, ntimes). So you can have a different number of traces than inputs, if you like.

@murrayrm
Copy link
Member Author

Presumably when the latter [plotting functionality] comes we can do things like add label text, which can come in handy.

I've added the ability to store labels in the latest commit and set things up so that the signals names for I/O systems, if present, are copied over as the default labels for input_output_response.

@sawyerbfuller
Copy link
Contributor

@billtubbs Would this PR help with what you were working on for #645 ? ie., should we merge this PR?

@murrayrm murrayrm marked this pull request as ready for review November 21, 2021 16:50
@murrayrm
Copy link
Member Author

I've been using the branch with this PR as my default branch for a while and everything seems OK. Ready to be merged, I think.

@billtubbs You mentioned in a separate note that you had made progress on #645. Does this conflict with that? If not, we can probably go ahead and merge.

@billtubbs
Copy link
Contributor

@billtubbs You mentioned in a separate note that you had made progress on #645. Does this conflict with that? If not, we can probably go ahead and merge.

I don't think so. I haven't looked at this in detail but I was only planning to fix the specific axes and figure problems with some of the plot functions rlocus, pzmap, bode_plot, nyquist_plot, gangof4_plot, nichols_plot. My work is probably already behind the master so will need revising anyway if there are any impacts.

@bnavigator bnavigator merged commit 56cecc0 into python-control:master Nov 21, 2021
@murrayrm murrayrm deleted the timeresponse_class branch November 25, 2021 22:06
@murrayrm murrayrm added this to the 0.9.1 milestone Dec 30, 2021
@murrayrm murrayrm mentioned this pull request Apr 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnavigator bnavigator bnavigator approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.
5 participants
@murrayrm @coveralls @sawyerbfuller @billtubbs @bnavigator