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 — Preserve signal names upon conversion to discrete-time by sawyerbfuller · Pull Request #797 · python-control/python-control

Conversation

@sawyerbfuller

@sawyerbfuller sawyerbfuller commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

This PR updates sys.sample in both StateSpace and TransferFunction to return a system with the same input and output labels, which is convenient when constructing interconnected systems using interconnect.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
sawyerbfuller added 2 commits November 22, 2022 21:30
@sawyerbfuller
initial commit that preserves signal names during cont-to-discrete tr…
055ed39
…ansformation
@sawyerbfuller
changed named signal handling to occur in sys.sample methods. added u…
1fd68c7
…nit tests.
@coveralls

coveralls commented Nov 23, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.01%) to 94.828% when pulling 16d9e6a on sawyerbfuller:named-signals into 2746ce1 on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

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

@murrayrm

murrayrm commented Nov 23, 2022

Copy link
Copy Markdown
Member

I suggest following the conventions used in linearize():

  • Use copy keyword to control whether signal names are copied (default can be True, although for linearize it is False for some reason).
  • Use name keyword to set the system name. If name is not specified and if copy is False, a generic name <sys[id]> is generated with a unique integer id. If copy is True, the new system name is determined by adding the prefix and suffix strings in config.defaults['iosys.linearized_system_name_prefix'] and config.defaults['iosys.linearized_system_name_suffix'], with the default being to add the suffix '$linearized'.

Doing this for sample(), the default name of the new system could be the old system with $sampled appended (and overridable via something like iosys.sample_system_name_prefix and iosys.sample_system_name_suffix.

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

All reactions

Sorry, something went wrong.

Uh oh!

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

sawyerbfuller added 3 commits November 23, 2022 10:51
@sawyerbfuller
create function to copy system names, move default name parameters to…
65e051f
… namedio, update sys.sample to enable signal names to be passed.
@sawyerbfuller
add unit tests and fixes to pass unit tests
28277f3
@sawyerbfuller
a few more unit tests and improvements to system name handling:
c345261
@sawyerbfuller

sawyerbfuller commented Nov 23, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@murrayrm thanks for the pointer. I refactored the code used in iosys.linearize to be a general-use method _copy_names that now lives in namedio that does the copying of various dicts correctly over to a new system.

Questions:

  1. I moved naming defaults associated with linearization and sampling to namedio because they are related to naming. OK?
  2. I changed the keyword to copy names to copy_names from copy in linearize and sample because I thought copy 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 new sample 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 the linearize docstrings but what actually happens is that copy 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 currently linearize has the old behavior of not copying over the names by default, but I am not sure if this is the best. sound reasonable?
All reactions

Sorry, something went wrong.

Uh oh!

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

@murrayrm

murrayrm commented Nov 24, 2022

Copy link
Copy Markdown
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 use kwargs to allow copy to still be used, but with a deprecation warning.
  • Rather than $copy, it seems like $sampled would be a better choice for sample_system().
  • We might want to create a copy method for NamedIOSystem and use the $copy suffix there.
All reactions

Sorry, something went wrong.

Uh oh!

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

@sawyerbfuller
simplify _copy_names, fix docstring errors, add names to sample_syste…
dbf64f2
…m and unit tests, namedio.copy is now deepcopy
@sawyerbfuller

sawyerbfuller commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

Great. New commit:

  • simplifies _copy_names to remove unnecessary functionality
  • sets default behavior to copy_names=True in iosys.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 accepts copy keyword, with a deprecation warning
  • namedio.copy now does a deepcopy instead of a shallow copy, addressing InputOutputSystem.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 in sys.sample
  • sample and sample_system append $sampled to the system name by default
All reactions

Sorry, something went wrong.

Uh oh!

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

@sawyerbfuller sawyerbfuller linked an issue Nov 24, 2022 that may be closed by this pull request
InputOutputSystem.copy makes shallow copies #728
Closed
@sawyerbfuller
fix deprecated functionality of copy keyword in iosys.linearize
16d9e6a
@murrayrm

murrayrm commented Nov 26, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

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

murrayrm
murrayrm approved these changes Nov 26, 2022
View reviewed changes
@murrayrm
murrayrm merged commit ed4ff84 into python-control:main Nov 26, 2022
@murrayrm murrayrm added this to the 0.9.3 milestone Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murrayrm murrayrm murrayrm approved these changes

Uh oh!

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

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

Uh oh!

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

3 participants

@sawyerbfuller @coveralls @murrayrm
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 01402503 — 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.