Skip to content

make _convert_to_statespace properly pass signal and system names #884

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

Merged
merged 3 commits into from
Jun 3, 2023

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfuller sawyerbfuller commented Apr 17, 2023

update: this PR has been converted to a bugfix in _convert_to_statespace to insure it copies signal and system names correctly. Found the bug by using the test suite.

Old PR description:
On some systems computers , the process of converting tf systems into LinearIOSystems in the interconnect function loses signal names -- if the transfer function is static. For example, if a system is defined as sys=tf(1, 2, inputs='a', outputs='b'), an error is raised when that system is interconnected with others because sys gets copied but loses its signal names, something along the lines of Signal 'a' not found.

But this doesn't happen on my system on Python 3.9, 3.10, or 3.11. Maybe something peculiar to windows?

This PR adds unit tests in an attempt to see if our test suite catches this bug.

…same as transfer functions. tests to track down missing names on some systems when converting to LinearIOSystem
@coveralls
Copy link

coveralls commented Apr 17, 2023

Coverage Status

Coverage: 94.535%. Remained the same when pulling 8b5eb47 on sawyerbfuller:static-signal-names into 1e1c8eb on python-control:main.

@sawyerbfuller
Copy link
Contributor Author

Found it! Losing signal names when slycot is not installed.

Working on a fix.

@sawyerbfuller sawyerbfuller changed the title PR aimed at finding missing signal names make _convert_to_statespace properly pass signal and system names Apr 18, 2023
@sawyerbfuller
Copy link
Contributor Author

Found the bug and squashed it. _convert_to_statespace was not passing signal names correctly in all cases, particularly with no slycot.

@murrayrm
Copy link
Member

murrayrm commented May 2, 2023

@sawyerbfuller Should the system name be copied over directly or should it be modified by appending $converted or something along those lines? One reason not to change the system name is that if you use interconnect with transfer functions, it is more natural to use the same system name. But this means that you can have two objects (the original TransferFunction and a StateSpace object) both having the same name...

@sawyerbfuller
Copy link
Contributor Author

@murrayrm my first thought is that the private method should keep the same system name because it is being used is internally.

But maybe if the system was changed by the user using ‘ss’, then that default should be overrided and the system should get a new name. sys$converted?

@murrayrm
Copy link
Member

I like the idea of having "$converted" appended when you do an explicit conversion (eg, via ss applied to a TransferFunction). That way you don't end up with two explicit objects floating around that have the same name but aren't the same object.

If we do that, we should probably generate a utility function/method that renames objects, probably in namedio.py so that we get some uniformity (including use of prefix and/or suffix, setting configuration defaults, etc). That goes beyond this PR, so could be split off as a separate issue?

@sawyerbfuller
Copy link
Contributor Author

HI @murrayrm that sounds good. Yes there are a few issues surrounding when to add $converted that may be outside of scope for this PR. E.g. if you call LinearIOSystem on a StateSpace system, does that get a suffix? if you call ss on a system, what method is responsible for adding $converted.

For this PR, OK if we leave it as it is, with _convert_to_statespace not changing the name? This is what you want when you are interconnecting TransferFunction and StateSpace objects and the former are being converted internally.

@murrayrm murrayrm merged commit 184d83d into python-control:main Jun 3, 2023
@murrayrm murrayrm added this to the 0.9.4 milestone Jun 7, 2023
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.

3 participants