Skip to content

fixed StateSpace for complex inputs #468

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fixed StateSpace for complex inputs #468

wants to merge 1 commit into from

Conversation

IguanaAzul
Copy link

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.

@bnavigator
Copy link
Contributor

Hi,

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

'statesp.use_numpy_matrix': True,
'statesp.use_numpy_matrix': False,
Copy link
Contributor

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.

Copy link
Contributor

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!

Copy link
Author

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

Copy link
Author

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?

@IguanaAzul
Copy link
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

@bnavigator
Copy link
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?

@bnavigator bnavigator closed this Nov 19, 2020
@bnavigator
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants