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 — fixed StateSpace for complex inputs by IguanaAzul · Pull Request #468 · python-control/python-control

Conversation

@IguanaAzul

@IguanaAzul IguanaAzul commented Nov 12, 2020

Copy link
Copy Markdown

The problem was that when trying to simulate a system with complex values in the state space matrices it was by default casting to float, which generated wrong results.

I had to create a pull request to scipy as well as the dlsim function has a similar issue:
scipy/scipy#13075

I also created an example script where this problem showed up and was fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@IguanaAzul
fixed StateSpace for complex inputs
b10f2af
@bnavigator

bnavigator commented Nov 12, 2020

Copy link
Copy Markdown
Contributor

Hi,

thanks for the contribution. Unfortunately the change is not straight forward. Are you aware of the discussion in #371, #372 and #376?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator
bnavigator reviewed Nov 12, 2020
View reviewed changes
Comment thread control/statesp.py
# Define module default parameter values
_statesp_defaults = {
'statesp.use_numpy_matrix': True,
'statesp.use_numpy_matrix': False,

@bnavigator bnavigator Nov 12, 2020

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.

Please don't change the default setting here. We have a road map in mind when this should happen and it should be independent of the support for complex types.

Sorry, something went wrong.

Uh oh!

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

@bnavigator bnavigator Nov 12, 2020

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.

That said, actually #438 takes complex inputs into account already!

Sorry, something went wrong.

Uh oh!

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

@IguanaAzul IguanaAzul Nov 12, 2020

Copy link
Copy Markdown
Author

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.

Ok, I will change this back

Sorry, something went wrong.

Uh oh!

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

@IguanaAzul IguanaAzul Nov 12, 2020

Copy link
Copy Markdown
Author

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.

That said, actually #438 takes complex inputs into account already!

Oh, I see, should I close my PR?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@IguanaAzul

IguanaAzul commented Nov 12, 2020

Copy link
Copy Markdown
Author

Hi,

thanks for the contribution. Unfortunately the change is not straight forward. Are you aware of the discussion in #371, #372 and #376?

Oh, I wasn`t aware of those discussions.
I think the fix I did conjugated with the fix on the scipy PR will work for input matrices with complex values.
scipy/scipy#13075
All reactions

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Nov 12, 2020

Copy link
Copy Markdown
Contributor

Travis is again not running, but without testing, I think your change will create a lot of casting warnings for cases, when users use control and expect real valued results. Given that #438 takes care of the correct type depending on the input, it should also resolve your use-case. That PR has merge-conflicts with current master right now, but are you able to check it out and test it anyway?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator closed this Nov 19, 2020
@bnavigator

bnavigator commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

I closed it because the tests fail and the issue is presumably resolved in another PR. Please feel free to open a new one if you think there is a better way than the approach in #438 or that your example script is a useful enhancement of the existing examples. But I honestly fail to see the point of the example beyond the fact that it demonstrates that python-control can't deal with complex state-space matrices yet.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator mentioned this pull request Jan 14, 2021
Release 0.8.4 and 0.9.0 sequencing #451
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnavigator bnavigator bnavigator left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

2 participants

@IguanaAzul @bnavigator
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 01402520 — 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.