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 — I/O system enhancements by murrayrm · Pull Request #1082 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Dec 20, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This PR provides a number of enhancements to I/O systems classes, most of which are internal but a few of which are visible to the user:

  • Updated factory functions to provide consistent functionality in terms of copying signal/system names, overriding system/signal names, and converting between classes.
  • Updated docstrings for all I/O classes and factory functions to provide consistent information and added unit tests to ensure consistency. The main changes were to make sure that all factory function parameters are documented while having the class docstrings focus on the primary parameters + attributes (with a reference to the factory function for more detailed documentation).
  • Updated the tf() factory function to allow a 2D list of SISO transfer functions to be given as a means of creating a MIMO transfer function (use the new combine_tf function). This is redundant with combine_tf, but seemed useful to include in the factory function for the special case of creating a MIMO transfer function via a set of SISO expressions. E.g.: 
    s = ct.tf('s')
sys = ct.tf([[1/s, 0], [(s+1)/(s**2 + 2s + 1), 1/(s+1)]])
  • [8f3615b, 22 Dec '24] Added processing of the params keyword to create_statefbk_iosystem.
  • [e4d373c, 22 Dec '24] Updated interconnect() to combine parameter lists of subsystems if params is not specified.
  • Changed some methods and attributes to be hidden so that they don't get flagged in docstring testing. These are functions that are not needed by the user (eg, FrequencyResponseData._ifunc, which is the internally generated interpolation function if the smooth keyword is used).
  • Changed the internal representation of transfer function numerators and denominators to a 2D array of arrays instead of a 2D nested list of arrays. This allows user of NumPy routines when accessing input/output pairs (eg, sys.num_array[i, j] instead of sys.num_list[i][j]). The nested list representation is still available via the {num,den}_list properties and the num and den attributes retrieve the nested list structure for backward compatibility. (We may want to change that in the future, but it could break user code where someone is accessing the numerator and denominator polynomials directly.)
  • Add the ability for nlsys to create a NonlinearIOSystem representation of a StateSpace system. It's not clear that this is so useful for a user (since StateSpace is a subclass of NonlinearIOSystem), but it is handy for testing since you can quickly create a nonlinear I/O system from a linear system but have it treated as a "pure" NonlinearIOSystem instead of a StateSpace system.
  • Got rid of redundant licensing information at the top of files that were modified (the information is contained in the LICENSE file in the main directory).

Note: this PR is likely to conflict with #1081, so the plan is to review/merge #1081 first, then rebase this PR on top of that one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TheStophe reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
murrayrm added 11 commits December 20, 2024 07:58
@murrayrm
add shape property to iosys
0003047
@murrayrm
update FRD to copy signal/system names on sampling
6c852c3
@murrayrm
convert internal representation of TF num/den to ndarray (vs list^2)
6df0919
@murrayrm
add tf() support for array of SISO [combine_tf] + documentation updates
9f8ff40
@murrayrm
change num/den and _num/_den to num/den_{list,array}
a7fd763
@murrayrm
updated iosys class/factory function documentation + docstring unit t…
2c7ca9a 
…esting
@murrayrm
ss -> nlsys with name preservation/override + docstring update
1547326
@murrayrm
update flatsys() documentation
319a8ea
@murrayrm
add unit test to insure all factory functions handle renaming
2f2e372
@murrayrm
document and fix {input,output,state}_prefix parameters
7a79342
@murrayrm
small docstring and unit test warning cleanup
34d5d03
@coveralls

coveralls commented Dec 20, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

coverage: 94.79% (+0.06%) from 94.73%
when pulling 1061445 on murrayrm:iosys_enhance-07Dec2024
into 9fa4aac on python-control:main.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@slivingston
slivingston self-requested a review December 20, 2024 21:54
murrayrm added 2 commits December 22, 2024 09:50
@murrayrm
add param processing to create_statefbk_iosystem
8f3615b
@murrayrm
update default params computation in interconnect()
e4d373c
@murrayrm
murrayrm force-pushed the iosys_enhance-07Dec2024 branch from 8888c03 to e4d373c Compare December 23, 2024 07:10
@slivingston

slivingston commented Dec 28, 2024

Copy link
Copy Markdown
Member

I will finish reviewing this tomorrow
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
slivingston
slivingston reviewed Dec 29, 2024
View reviewed changes

@slivingston slivingston left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made several comments about misprints. Code appears to be correct. I am going to examine it a bit more in the morning.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/tests/docstrings_test.py Outdated
#
# List of arguments described in class docstrings
#
# These are the minimal arguments needed to initialized the class. Optional

@slivingston slivingston Dec 24, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# These are the minimal arguments needed to initialized the class. Optional
# These are the minimal arguments needed to initialize the class. Optional

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/tests/docstrings_test.py Outdated
# List of attributes described in class docstrings
#
# This is the list of attributes for the class that are not already listed
# as parameters used to inialize the class. These should all be defined

@slivingston slivingston Dec 24, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# as parameters used to inialize the class. These should all be defined
# as parameters used to initialize the class. These should all be defined

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/frdata.py Outdated
frequency : 1D array
Array of frequency points for which data are available.
ninputs, noutputs : int
Number of inputs and outputs signals.

@slivingston slivingston Dec 24, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Number of inputs and outputs signals.
Number of input and output signals.

which is consistent with usage below (description of input_labels, output_labels), or could be "Number of inputs and outputs."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/frdata.py Outdated
magnitude : array
Magnitude of the frequency response, indexed by frequency.
phase : array
Magnitude of the frequency response, indexed by frequency.

@slivingston slivingston Dec 24, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Magnitude of the frequency response, indexed by frequency.
Phase of the frequency response, indexed by frequency.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
-----
The attribues 'num' and 'den' are 2-D lists of arrays containing MIMO
numerator and denominator coefficients. For example,
The numerator and denominator polynomials are stored as 2D ndarray's

@slivingston slivingston Dec 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The numerator and denominator polynomials are stored as 2D ndarray's
The numerator and denominator polynomials are stored as 2D ndarrays

Style used in the NumPy documentation is "ndarrays" (no apostrophe); e.g., in https://numpy.org/doc/stable/user/basics.indexing.html and https://numpy.org/doc/stable/user/basics.creation.html.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/statesp.py Outdated
#
# Author: Richard M. Murray
# Date: 24 May 09
# Revised: Kevin K. Chen, Dec 10

@slivingston slivingston Dec 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside the scope of this PR, but eventually we should consider removing comments with author names like this or add a note that

  • these dates are from before moving to Git for version control
  • and the up-to-date authorship is available in commit history.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@murrayrm murrayrm Dec 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a cut at this in the commit that is coming next. See what you think and we can fix things up globally (I have another PR in the works where I could do that).

Sorry, something went wrong.

Uh oh!

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

@slivingston slivingston Dec 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your proposed change: it is concise and shows a simple command to get the list of authors.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
to prettier LaTeX representation """
"""Superscript all digits in a polynomial string and convert float
coefficients in scientific notation to prettier LaTeX
representation.

@slivingston slivingston Dec 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring alignment is odd for this function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
update to address 29 Dec comments by @slivingston
e10d481
slivingston
slivingston reviewed Jan 4, 2025
View reviewed changes
Comment thread control/iosys.py Outdated
Comment on lines +154 to +155
repr_format : str
String representation format ('iosys' or 'loadable').

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a repr_format attribute, or is this referring to __repr__? If the latter, I find it confusing to write "repr_format" in this list because it is not the name of an attribute. Also, note that __repr__ is a standard method described in the Python documentation, so probably OK to not document it here.

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 4, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this crept through from a separate PR that I am working on (and will show up next). Will delete from here (and one other spot where it shows up).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/tests/xferfcn_test.py Outdated
Comment on lines 34 to 37
# 13 Dec 2024: This now works correctly: creates static array (as tf)
# with pytest.raises(TypeError):
# TransferFunction([[0., 1.], [2., 3.]], [[5., 2.], [3., 0.]])
# good input

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend deleting this comment block (from "13 Dec..." to "good input"). The fact that this was previously tested as a known error is already recorded in commit history. If the goal with this comment is to notify users about the behavior change, then probably it will be easier to find in the next release announcement.

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 4, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this test completely, since it is now longer a "bad input type" (from the name of the test method).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py
Comment on lines +222 to 225
for arr in [num, den]:
for poly in np.nditer(arr, flags=['refs_ok']):
if poly.item().size > 1:
static = False

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for arr in [num, den]:
for poly in np.nditer(arr, flags=['refs_ok']):
if poly.item().size > 1:
static = False
for arr in [num, den]:
if not static:
break
for poly in np.nditer(arr, flags=['refs_ok']):
if poly.item().size > 1:
static = False
break

This is a small optimization: I noticed that after static = False is reached, the outer and inner loops can be exited immediately. For example, the system in the PR description:

s = ct.tf('s')
sys = ct.tf([[1/s, 0], [(s+1)/(s**2 + 2*s + 1), 1/(s+1)]])

results in several cases of needlessly reaching static = False more than once. However, I did not rigorously quantify the potential benefit, so OK to not do this.

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 4, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 4, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update coming in a bit to address these.

@slivingston Are you still reviewing, or is it OK to merge? (I have two more PR's coming that build on this one.)

Sorry, something went wrong.

Uh oh!

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

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@murrayrm Sorry for the delay. I am still reviewing but will be done in 30 minutes.

Sorry, something went wrong.

Uh oh!

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

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@murrayrm done now. There are 2 misprints and 1 bug. After handling these, ready to merge.

Sorry, something went wrong.

Uh oh!

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

@sawyerbfuller sawyerbfuller Jan 4, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR. Just came here to note that there is already an _isstatic method that is also short-circuiting. Can we use the same code for both - perhaps by moving the code to a separate function that tests num and den, and a method that calls it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
Remove repr_format mentions; fix xferfcn static check per @slivingston
bf19193
slivingston
slivingston reviewed Jan 4, 2025
View reviewed changes
Comment thread control/nlsys.py Outdated
Comment on lines +1336 to +1337
lambda t, x, u, params: sys.A @ x + sys.B @ u,
lambda t, x, u, params: sys.C @ x + sys.D @ u, **kwargs)

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lambda t, x, u, params: sys.A @ x + sys.B @ u,
lambda t, x, u, params: sys.C @ x + sys.D @ u, **kwargs)
lambda t, x, u, params: sys_ss.A @ x + sys_ss.B @ u,
lambda t, x, u, params: sys_ss.C @ x + sys_ss.D @ u, **kwargs)

Sorry, something went wrong.

Uh oh!

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

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug and needs to be fixed before merging.

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Jan 4, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on this. In the latest update, I added a unit test that catches the bug (and fixed the bug).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
slivingston
slivingston reviewed Jan 4, 2025
View reviewed changes
Comment thread control/statefbk.py Outdated

params : dict, optional
System parameter values. By default, these will be copied from
`sys` and `ctrl`, but can be overriden with this keyword.

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`sys` and `ctrl`, but can be overriden with this keyword.
`sys` and `ctrl`, but can be overridden with this keyword.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/statesp.py Outdated

``ss(args, inputs=['u1', ..., 'up'], outputs=['y1', ..., 'yq'], states=['x1', ..., 'xn'])``
``ss(*args, inputs=['u1', ..., 'up'], outputs=['y1', ..., 'yq'],
states=['x1', ..., 'xn'])``

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this in-line code expression across 2 lines breaks Sphinx processing, resulting in
image
Compare with the rendering of ss(A, B, C, D, dt) in the above image and the rendering of these in the current docs at https://python-control.readthedocs.io/en/0.10.1/generated/control.ss.html

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py
Comment on lines +222 to 225
for arr in [num, den]:
for poly in np.nditer(arr, flags=['refs_ok']):
if poly.item().size > 1:
static = False

@slivingston slivingston Jan 4, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@murrayrm Sorry for the delay. I am still reviewing but will be done in 30 minutes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
addressed @slivingston review comments
1061445
@murrayrm
murrayrm merged commit 93c4c8d into python-control:main Jan 4, 2025
@murrayrm murrayrm mentioned this pull request Jan 4, 2025
Remove redundant _isstatic calculation in xferfcn #1090
Closed
@murrayrm murrayrm added this to the 0.10.2 milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slivingston slivingston slivingston left review comments
@sawyerbfuller sawyerbfuller sawyerbfuller left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

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