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

Preserve signal names upon conversion to discrete-time#797

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
murrayrm merged 7 commits intopython-control:mainfromsawyerbfuller:named-signals
Nov 26, 2022

Conversation

sawyerbfuller
Copy link
Contributor

This PR updatessys.sample in bothStateSpace andTransferFunction to return a system with the same input and output labels, which is convenient when constructing interconnected systems usinginterconnect.

The changes in this PR only copy signal names, but the name of the system is not preserved. I haven't used the system name feature before but my impression was that it should be unique, but please let me know if you think it should be passed as well.

@coveralls
Copy link

coveralls commentedNov 23, 2022
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 94.828% when pulling16d9e6a on sawyerbfuller:named-signals into2746ce1 on python-control:main.

@murrayrm
Copy link
Member

I suggest following the conventions used inlinearize():

  • Usecopy keyword to control whether signal names are copied (default can be True, although forlinearize it is False for some reason).
  • Usename keyword to set the system name. Ifname is not specified and ifcopy isFalse, a generic name <sys[id]> is generated with a unique integer id. Ifcopy isTrue, the new system name is determined by adding the prefix and suffix strings inconfig.defaults['iosys.linearized_system_name_prefix'] andconfig.defaults['iosys.linearized_system_name_suffix'], with the default being to add the suffix '$linearized'.

Doing this forsample(), the default name of the new system could be the old system with$sampled appended (and overridable via something likeiosys.sample_system_name_prefix andiosys.sample_system_name_suffix.

Also, we might want to override thesample() method inLinearIOSystem so that sampling a linear input/output system gives a linear input/output system (rather than just aStateSpace system). This won't matter much sinceStateSpace is converted toLinearIOSystem if it is connected to anything, but would be a bit cleaner (and points to the fact that we should probably mergeStateSpace andLinearIOSystem at some point, as pointed out already in issue#786).

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedNov 23, 2022
edited
Loading

@murrayrm thanks for the pointer. I refactored the code used iniosys.linearize to be a general-use method_copy_names that now lives innamedio that does the copying of various dicts correctly over to a new system.

Questions:

  1. I moved naming defaults associated with linearization and sampling tonamedio because they are related to naming. OK?
  2. I changed the keyword to copy names tocopy_names fromcopy inlinearize andsample because I thoughtcopy might somehow suggest copying the whole system somehow and this better represented what it should do. This does (slightly) break backward compatibility. OK?
  3. the newsample allows copying the names but also supplying new keyword arguments that additionally allow those signal labels to be overridden. This is suggested for how it works in thelinearize docstrings but what actually happens is thatcopy ignored any keyword-supplied names. I changed this so that supplied names override copied names. OK?
  4. sample copies the names over by default because that is how they are likely to be used, but currentlylinearize has the old behavior of not copying over the names by default, but I am not sure if this is the best. sound reasonable?

@murrayrm
Copy link
Member

I like the revisions and pretty much agree with them all, especially moving the functionality up to namedio.py (which is where it belongs). Some additional thoughts:

  • One way to preserve backward compatibility would be to usekwargs to allowcopy to still be used, but with a deprecation warning.
  • Rather than$copy, it seems like$sampled would be a better choice forsample_system().
  • We might want to create acopy method forNamedIOSystem and use the$copy suffix there.

…m and unit tests, namedio.copy is now deepcopy
@sawyerbfuller
Copy link
ContributorAuthor

Great. New commit:

  • simplifies_copy_names to remove unnecessary functionality
  • sets default behavior tocopy_names=True iniosys.linearize. happy to revert it if you think it's better not to.
  • fix docstring errors
  • more unit tests

Following@murrayrm's comment above:

  • linearize now acceptscopy keyword, with a deprecation warning
  • namedio.copy now does a deepcopy instead of a shallow copy, addressingInputOutputSystem.copy makes shallow copies #728. Before, all of its dicts and arrays were just references to objects that could be modified. The new system has$copy appended to its name, as before.
  • dtime.sample_system now gets all of the naming functionality that is now present insys.sample
  • sample andsample_system append$sampled to the system name by default

@sawyerbfullersawyerbfuller linked an issueNov 24, 2022 that may beclosed by this pull request
@murrayrm
Copy link
Member

This all looks good to me. Will let it sit for another day or two just in case someone else wants to take a look, but I think it is ready to merge.

@murrayrmmurrayrm merged commited4ff84 intopython-control:mainNov 26, 2022
@murrayrmmurrayrm added this to the0.9.3 milestoneDec 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@murrayrmmurrayrmmurrayrm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.3
Development

Successfully merging this pull request may close these issues.

InputOutputSystem.copy makes shallow copies
3 participants
@sawyerbfuller@coveralls@murrayrm

[8]ページ先頭

©2009-2025 Movatter.jp