- Notifications
You must be signed in to change notification settings - Fork446
Support for binary operations between MIMO and SISO LTI systems#1081
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fix scalar tfops 06 dec2024
…sting for SS __mul__ and __rmul__
coveralls commentedDec 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
murrayrm commentedDec 19, 2024
I'll try to review this in the coming days. In the meantime, there is a typo in a docstring ("ct.ombine_tf" should be "ct.combine_tf") that is causing a failure in the doctest check. |
sdahdah commentedDec 20, 2024
Thank you! Is there a way to run the doctests locally? |
murrayrm commentedDec 20, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
make doctest in the doc/ directory will run the tests. |
murrayrm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Preliminary comments. I haven't gone through to make sure the everything is correct, but wanted to pass on some higher level comments first. OK to ignore the code style ones, but I would fix up some of the Numpydoc ones (which I eventually need to check intests/docstrings_tests.py.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
sdahdah commentedDec 21, 2024
Thanks for taking a look. Would you like me to fix these before you continue the review? I might not be able to until the new year unfortunately. |
murrayrm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As currently written, the new systems that are being created are non-minimal. For example:
s = ct.tf('s')ct.config.defaults['xferfcn.display_format'] = 'zpk' # simpler output formatsiso = (s+1) / ((s+2) * (s+3))mimo = ct.combine_tf([ [1/s, 1/(s+4)], [1/(s+5), (s+6)/(s**2 + 2*s + 2)]])print(siso * mimo)<TransferFunction>: sys[219]Inputs (2): ['u[0]', 'u[1]']Outputs (2): ['y[0]', 'y[1]']Input 1 to output 1: (s + 1) (s + 5)---------------------------(s) (s + 2) (s + 3) (s + 5)Input 1 to output 2: (s) (s + 1)---------------------------(s) (s + 2) (s + 3) (s + 5)Input 2 to output 1: (s + 1) (s + (1-1j)) (s + (1+1j))-------------------------------------------------(s + (1-1j)) (s + (1+1j)) (s + 2) (s + 3) (s + 4)Input 2 to output 2: (s + 1) (s + 4) (s + 6)-------------------------------------------------(s + (1-1j)) (s + (1+1j)) (s + 2) (s + 3) (s + 4)Note the various pole/zero cancellations that appear. It seems to me that these are spurious and shouldn't show up (eg, (s+4) in the [2, 2] transfer function).
This same type of situation occurs in state space and is consistent with the results that occur if you convert the SISO system to a matrix system by hand. I'm not sure (yet) what is gong on, but this seems like something we should fix, either for this specific case or for the more general case of multiplying MIMO systems.
I need to think a bit more about what is causing this, but wanted to flag it here in case someone else has some insights. It seems to me that perfect pole/zero cancellations (and the equivalent in state space) should get sorted out ahead of time, either by not creating them or by removing them before we send things back to the user.
Uh oh!
There was an error while loading.Please reload this page.
sdahdah commentedJan 17, 2025
I took a quick look but I'm not sure. It's definitely happening inside |
sdahdah commentedJan 17, 2025
Happy new year@murrayrm . I fixed some of your initial comments (and also tried to look for other instances of nonstandard docstrings and indentation along the way). I'm not 100% sure what's going on with the spurious poles and zeros, buy maybe another issue should be raised to track that? |
sdahdah commentedJan 17, 2025
Not sure why the I don't think it's related to the PR |
71bd731 intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Resolves#1076 and#939
This PR implements binary operations (
__add__,__mul__, etc.) between MIMO and SISO LTI systems.Specifically, it
TransferFunction.append()andFrequencyResponseData.append(),control.append()to return the type of the first argument,__mul__,__rmul__,__truediv__and__rtruediv__for MIMO and SISO LTI systems using the updatedcontrol.append(),__add__,__radd__,__sub__, and__rsub__using the updated multiplication and division operations,__pow__, andStateSpace.minreal()drops thedtI would recommend reviewing the changes in that order.
See also#459