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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfullersawyerbfuller commentedApr 17, 2023
edited
Loading

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 somesystems computers , the process of convertingtf systems intoLinearIOSystems in theinterconnect function loses signal names -- if the transfer function is static. For example, if a system is defined assys=tf(1, 2, inputs='a', outputs='b'), an error is raised when that system is interconnected with others becausesys gets copied but loses its signal names, something along the lines ofSignal '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 commentedApr 17, 2023
edited
Loading

Coverage Status

Coverage: 94.535%. Remained the same when pulling8b5eb47 on sawyerbfuller:static-signal-names into1e1c8eb on python-control:main.

@sawyerbfuller
Copy link
ContributorAuthor

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

Working on a fix.

@sawyerbfullersawyerbfuller changed the titlePR aimed at finding missing signal namesmake _convert_to_statespace properly pass signal and system namesApr 18, 2023
@sawyerbfuller
Copy link
ContributorAuthor

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

@sawyerbfuller Should the system name be copied over directly or should it be modified by appending$converted or something along those lines? One reasonnot to change the system name is that if you useinterconnect 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
ContributorAuthor

@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, viass applied to aTransferFunction). 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 innamedio.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
ContributorAuthor

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 callLinearIOSystem on aStateSpace system, does that get a suffix? if you callss 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 interconnectingTransferFunction andStateSpace objects and the former are being converted internally.

@murrayrmmurrayrm merged commit184d83d intopython-control:mainJun 3, 2023
@murrayrmmurrayrm added this to the0.9.4 milestoneJun 7, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.4
Development

Successfully merging this pull request may close these issues.

3 participants
@sawyerbfuller@coveralls@murrayrm

[8]ページ先頭

©2009-2025 Movatter.jp