Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
murrayrm merged 42 commits intopython-control:mainfromsdahdah:main
Jan 20, 2025

Conversation

sdahdah
Copy link
Contributor

@sdahdahsdahdah commentedDec 19, 2024
edited
Loading

Resolves#1076 and#939

This PR implements binary operations (__add__,__mul__, etc.) between MIMO and SISO LTI systems.

Specifically, it

  • implementsTransferFunction.append() andFrequencyResponseData.append(),
  • changescontrol.append() to return the type of the first argument,
  • implements MIMO-SISO__mul__,__rmul__,__truediv__ and__rtruediv__ for MIMO and SISO LTI systems using the updatedcontrol.append(),
  • implements MIMO-SISO__add__,__radd__,__sub__, and__rsub__ using the updated multiplication and division operations,
  • implements inversion of biproper state-space systems via__pow__, and
  • fixes a bug (?) whereStateSpace.minreal() drops thedt

I would recommend reviewing the changes in that order.

See also#459

murrayrmand others added30 commitsDecember 6, 2024 17:36
@coveralls
Copy link

coveralls commentedDec 19, 2024
edited
Loading

Coverage Status

coverage: 94.645% (-0.04%) from 94.687%
when pullingccf9ce1 on sdahdah:main
into0ff0452 on python-control:main.

@murrayrm
Copy link
Member

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 reacted with thumbs up emoji

@murrayrmmurrayrm self-assigned thisDec 19, 2024
@murrayrmmurrayrm self-requested a reviewDecember 19, 2024 22:41
@murrayrmmurrayrm removed their assignmentDec 19, 2024
@sdahdah
Copy link
ContributorAuthor

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.

Thank you! Is there a way to run the doctests locally?make html in thedoc/ folder I think? I have not been able to figure that out

@murrayrm
Copy link
Member

murrayrm commentedDec 20, 2024
edited
Loading

Is there a way to run the doctests locally?make html in thedoc/ folder I think? I have not been able to figure that out

make doctest in the doc/ directory will run the tests.

sdahdah reacted with thumbs up emoji

@murrayrmmurrayrm mentioned this pull requestDec 20, 2024
Copy link
Member

@murrayrmmurrayrm left a 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.

@sdahdah
Copy link
ContributorAuthor

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.

Copy link
Member

@murrayrmmurrayrm left a 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.

sdahdah reacted with thumbs up emoji
@sdahdah
Copy link
ContributorAuthor

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.

I took a quick look but I'm not sure. It's definitely happening insidexferfcn.TransferFunction.__mul__. I think roots need to be cancelled inxferfcn._add_siso(), but I'm not sure whetherminreal() should just be used?

@sdahdah
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

Not sure why thePy3.12; conda Slycot; conda Pandas; conda CVXOPT ; QtAgg check is failing during setup:

error    libmamba Error when extracting package: Could not stat include/event2/keyvalq_struct.hlibevent-2.1.12-hf998b51_1.conda extraction failedFound incorrect download: libevent. Aborting

I don't think it's related to the PR

@murrayrmmurrayrm merged commit71bd731 intopython-control:mainJan 20, 2025
23 checks passed
@murrayrmmurrayrm added this to the0.10.2 milestoneFeb 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@murrayrmmurrayrmAwaiting requested review from murrayrm

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

Successfully merging this pull request may close these issues.

Proposal for multiplying a NumPy array by a scalarTransferFunction
3 participants
@sdahdah@coveralls@murrayrm

[8]ページ先頭

©2009-2025 Movatter.jp