GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix interconnect type conversion bug for StateSpace systems by murrayrm · Pull Request #788 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Nov 13, 2022

Copy link
Copy Markdown
Member

This PR fixes a problem that was identified in PR #785, where interconnecting a LinearIOSystem with a StateSpace system via the interconnect function did not work correctly. In particular, if you created a mixed system of this type you would get back an InterconnectedSystem that would generate an error is you tried to simulate it or evaluate the dynamics. This was fixed by adding a few lines of code to interconnect() that convert StateSpace and TransferFunction objects to LinearIOSystems, mimicking what is done with operator overloading.

In addition, there was a bug where the param keyword was not allowed in the dynamics and output functions. This is now fixed and tested with a unit test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
murrayrm added 2 commits November 12, 2022 15:45
@murrayrm
add conversions for LTI systems in interconnect()
45c3abd
@murrayrm
fix params processing in I/O system + unit tests
43ce56b
@murrayrm murrayrm mentioned this pull request Nov 13, 2022
fix to insure that when a StateSpace and a LinearIOSystem are combined, the result is a LinearIOSystem #785
Closed
@murrayrm murrayrm changed the title Interconnect type conversion 12 nov2022 Fix interconnect type conversion bug for StateSpace systems Nov 13, 2022
@coveralls

coveralls commented Nov 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 94.801% when pulling a9a6226 on murrayrm:interconnect_type_conversion-12Nov2022 into beb629b on python-control:main.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@murrayrm
update params docstrings in iosys
1a78cb8
roryyorke
roryyorke reviewed Nov 13, 2022
View reviewed changes

@roryyorke roryyorke left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM, except for the one comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/iosys.py Outdated
type(self))

def dynamics(self, t, x, u):
def dynamics(self, t, x, u, params={}):

@roryyorke roryyorke Nov 13, 2022

Copy link
Copy Markdown
Contributor

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.

Is there a reason not to use the params=None, if params is None: params={} pattern here? Using mutable objects for default parameters is usually not a good idea.

Also, why not **kwargs?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Nov 13, 2022
View reviewed changes
Comment thread control/iosys.py Outdated
returns the time derivative

dx/dt = f(t, x, u)
dx/dt = f(t, x, u, params)

@sawyerbfuller sawyerbfuller Nov 13, 2022

Copy link
Copy Markdown
Contributor

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.

Suggested change
dx/dt = f(t, x, u, params)
dx/dt = f(t, x, u[, params])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Nov 13, 2022
View reviewed changes
Comment thread control/iosys.py Outdated
If the system is discrete-time, returns the next value of `x`:

x[t+dt] = f(t, x[t], u[t])
x[t+dt] = f(t, x[t], u[t], params)

@sawyerbfuller sawyerbfuller Nov 13, 2022

Copy link
Copy Markdown
Contributor

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.

Suggested change
x[t+dt] = f(t, x[t], u[t], params)
x[t+dt] = f(t, x[t], u[t][, params])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Nov 13, 2022
View reviewed changes

@sawyerbfuller sawyerbfuller left a comment

Copy link
Copy Markdown
Contributor

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.

One other thought: currently, StateSpace.dynamics does not have params as a kwarg, but it probably should (maybe with a warning that it will be ignored).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
murrayrm added 2 commits November 12, 2022 23:12
@murrayrm
replace mutables as arguments with None, per @roryyorke suggestion
878cf98
@murrayrm
update params docstrings + warnings per @sawyerbfuller suggestion
a9a6226
@roryyorke

roryyorke commented Nov 13, 2022

Copy link
Copy Markdown
Contributor

Like the for x in sequence or []: pattern. LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller
sawyerbfuller merged commit b32e355 into python-control:main Nov 16, 2022
@murrayrm murrayrm added this to the 0.9.3 milestone Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sawyerbfuller sawyerbfuller sawyerbfuller left review comments
+1 more reviewer
@roryyorke roryyorke roryyorke left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.3

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@murrayrm @coveralls @roryyorke @sawyerbfuller
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.