GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||
GeezerVison is showing github.com — Add `combine()` and `split()` functions for transfer matrices by sdahdah · Pull Request #1073 · python-control/python-control Conversation
Contributor
RE #325 I've run into a few times where it would be convenient to construct MIMO I saw there was a related issue (#325 ) and people seemed open to it. Specifically, I've added
I'm new to the
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
|
coverage: 94.723% (+0.03%) from 94.689% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! This will make a nice addition.
Lots of little suggestions herein, mainly to keep things consistent with other code in the package.
One bigger picture question is on the function names: since these only work for transfer functions, I wonder if the function names shouldn't be combine_tf and split_tf (using 'tf' rather than transfer_function since the factor function is tf).
Also, I think there needs to be some checking for size consistency. For example, consider the following code:
import control as ct
sys1 = ct.tf(ct.rss(2, 1, 2))
sys2 = ct.tf(ct.rss(2, 2, 1))
sys = ct.combine([[sys1, sys2]])
The first system has 1 output and the second system has two outputs, so I think this should generate an error. Instead, I get a 3 input (correct) by 1 output (incorrect) system. If instead I do
sys = ct.combine([[sys2, sys1]])
I get an error message:
587 for col in row:
588 for j_in in range(col.ninputs):
--> 589 num_row.append(col.num[j_out][j_in])
590 den_row.append(col.den[j_out][j_in])
591 num.append(num_row)
IndexError: list index out of range
Shouldn't both forms generate error messages (perhaps saying that there are an inconsistent number of outputs in row 1)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| try: | ||
| for row in tf_array: | ||
| for tfn in row: | ||
| dt_list.append(getattr(tfn, "dt", None)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
dt will always exist for an InputOutputSystem, so this can just be tfn.dt.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The combine_tf() function allows NumPy arrays to be input as well, so you can do something like
control.combine_tf([ [np.eye(2), np.zeros((2, 1))], [np.zeros((1, 2)), control.TransferFunction([1], [1, 0])], ])
So in that case, the NumPy arrays do not have a dt attribute. The dt will be taken from the TransferFunction object in this case.
I've found this useful when creating weighting functions for optimal control problems.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks for the feedback. Adding I've also fixed the dimension checking and added a couple unit tests for that case I missed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.