-
Notifications
You must be signed in to change notification settings - Fork 439
Update I/O system repr() and str() #1091
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
Conversation
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.
Nice PR. Showing signal names by default in notebooks will help my students/users catch a lot of bugs.
Is the different format for the inputs for stateless systems u0
, u1
vs u[0]
, u[1]
for other systems a bug or a feature? Maybe a bug that this PR uncovered.
The input names appear for that example because the inputs were given names when the system was created(and so they show up explicitly to preserve that naming). |
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.
A bit odd for me to be reviewing my own PR, but this is a pretty big PR and it built on top of another one, so I wanted to do one last check. I found a couple of things to look into.
control/config.py
Outdated
# Verions 0.10.2 | ||
if major == 0 and minor <= 10 and patch < 2: | ||
set_defaults('iosys', repr_format='eval') | ||
|
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.
Current release also uses eval
as default, so this may not be needed. We could set iosys.show_count
to False here, to match v0.10.1, but it seems like including the counts is never something that is going to cause a problem in running code.
escape_chars = { | ||
'$': r'\$', | ||
'<': '<', | ||
'>': '>', |
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.
It might look better to change the -> in the I/O system representation into
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 left this alone but added a comment to enhance this in a future release.
control/iosys.py
Outdated
|
||
Notes | ||
----- | ||
By default, the representation for an input/output is set to 'info'. |
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 is now 'eval'; see top of file.
control/nlsys.py
Outdated
@@ -154,9 +154,13 @@ def __init__(self, updfcn, outfcn=None, params=None, **kwargs): | |||
self._current_params = {} if params is None else params.copy() | |||
|
|||
def __str__(self): | |||
return f"{InputOutputSystem.__str__(self)}\n\n" + \ | |||
out = f"{InputOutputSystem.__str__(self)}" | |||
if len(self.params) > 1: |
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.
Shouldn't this be > 0?
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.
Fixed and added a unit test that exhibits the (now corrected) error.
# Use StateSpace.__call__ to evaluate at a given complex value | ||
def __call__(self, *args, **kwargs): | ||
return StateSpace.__call__(self, *args, **kwargs) |
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.
Since this was apparently incorrect before, is there a missing unit test?
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.
Fixed (indentation was wrong) and added unit test. Turns out the original version worked because it picked the right parent class (StateSpace versus NonlinearIO)
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 need 1 day more to finish reviewing. Sending the small misprints I noticed thus far.
examples/repr_gallery.py
Outdated
# of representations (__repr__, __str__) for those systems that can be | ||
# used to compare different versions of python-control. It is mainly | ||
# intended for uses by developers to make sure there are no unexpected | ||
# changes in representation formats, but also has some interest |
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.
# changes in representation formats, but also has some interest | |
# changes in representation formats, but also has some interesting |
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.
The suggested change applies to repr_gallery.ipynb, too.
examples/repr_gallery.py
Outdated
sys_gtf = ct.tf([1], [1, 0]) | ||
syslist += [sys_tf, sys_dtf, sys_gtf] | ||
|
||
# MIMO transfer function (continous time only) |
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.
# MIMO transfer function (continous time only) | |
# MIMO transfer function (continuous time only) |
examples/repr_gallery.py
Outdated
name='sys_mtf_zpk', display_format='zpk') | ||
syslist += [sys_mtf] | ||
|
||
# Frequency response data (FRD) system (continous and discrete time) |
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.
# Frequency response data (FRD) system (continous and discrete time) | |
# Frequency response data (FRD) system (continuous and discrete time) |
control/config.py
Outdated
@@ -311,6 +333,10 @@ def use_legacy_defaults(version): | |||
# | |||
reset_defaults() # start from a clean slate | |||
|
|||
# Verions 0.10.2 |
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.
# Verions 0.10.2 | |
# Version 0.10.2 |
control/tests/nlsys_test.py
Outdated
@@ -19,7 +19,7 @@ | |||
# Basic test of nlsys() | |||
def test_nlsys_basic(): | |||
def kincar_update(t, x, u, params): | |||
l = params.get('l', 1) # wheelbase | |||
l = params['l'] # wheelbase |
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.
l = params['l'] # wheelbase | |
l = params['l'] # wheelbase |
1 space more to align with "x velocity" on line 24.
control/tests/iosys_test.py
Outdated
sys.repr_format = format | ||
out = repr(sys) | ||
if format == 'eval': | ||
assert re.search(expected, out) != None |
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.
assert re.search(expected, out) != None | |
assert re.search(expected, out) is not None |
Preferred style (in PEP 8) is is not None
instead of != None
. This comment applies to != None
on lines 2370 and 2348, too.
control/tests/config_test.py
Outdated
sys = ct.ss([[1]], [[1]], [[1]], [[0]]) | ||
new = eval(repr(sys)) | ||
for attr in ['A', 'B', 'C', 'D']: | ||
assert getattr(sys, attr) == getattr(sys, attr) |
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.
assert getattr(sys, attr) == getattr(sys, attr) | |
assert getattr(sys, attr) == getattr(new, attr) |
@slivingston Thanks for the detailed review. Let me know if additional comments are coming, otherwise I'll go head and merge. |
b44d5ba
to
0f0fad0
Compare
I will do one more look now and wrap it up within 2 hours. |
This PR provides updates to the string representations of I/O systems, including both the
__repr__
and__str__
functions (used forrepr()
,str()
, andprint()
) as well as LaTeX output (via_repr_html_
):_repr_*
formats forInputOutputSystem
's that can be overridden in subclasses to get the most useful representations based on the system type, using method names similar to_repr_latex_
used by Jupyter notebooks:_repr_info_
: generates a minimal description of the I/O system in the form<SystemClass name: [input list] -> [output list][, dt=timebase]>
. This is the default format that exists for all I/O systems. The inclusion of the timebase is new._repr_eval_
: generates an "evaluatable" description of the I/O system, if possible (otherwise return_repr_info_()
). This is used inLTI
systems to show the state space matrices, numerator/denominator, or freuquency response data. This format is similar to what was done before this PR forLTI
classes, but now includes the signal names (if non-generic) or the signal counts (if generic). The inclusion of the string names make sure that you can recreate the system with the proper labels. The inclusion of the string counts lets you see the number of inputs and outputs._repr_html_
: generates an HTML/LaTeX description of the I/O system, if possible (otherwise return_repr_info_()
). This method is picked up by Jupyter notebooks to display the value of a variable and replaces_repr_latex_
used in prior versions. This is similar to what was present before, except that now there is a "header" that shows the system name and signal names before showing a LaTeX representation of the state space matrices and/or transfer functions (this required changing_repr_latex_
to_repr_html_
). Note that since the timebase information is included in the header, it is no longer included in the LaTeX formatted code.iosys_repr
(similar tonp.array_repr
) that allows different representations of I/O systems via aformat
parameter ('info', 'eval', or 'latex').display_format
parameter is now handled in a dynamic fashion. If it is set toNone
(default) on system creation, the display style will followconfig.defaults['xferfcn.display_format']
. If set topoly
orzpk
on system creation, that overrides the default.str()
(print) representation ofInputOutputSystems
to includedt
after the state/input/output names (suppressed if it matches the defaultdt
).str()
(print) representation forNonlinearIOSystems
to include list of parameter labels.str()
(print) representation forInterconnectedSystems
to include the list of subsystems (in 'info' form) as well summaries of the connections and outputs matrix (might be an eventual replacement forconnection_table
).np.printoptions
to system arrays before generating strings, so you get consistent formatting across representations (including latex) and you can use things likesuppress
to display near-zero numbers as '0'.str()
(print) to make things consistent across the various I/O system classes, including adding some indentation for transfer functions and MIMO systems (see screen shots below).examples/repr_gallery.ipynb
andexampes/repr_gallery.py
files to show all of the various output formats and options (with dashed lines at the start and end so you can see any extraneous blank lines).config.defaults
options 'iosys.repr_format' and 'iosys.repr_show_count' to control representation formats.combine_tf
function now accepts system and signal name keywords. (This is not strictly related to the main subject of this PR, but I needed since I had to set the name of the system in order for doctests to work.)config.defaults
to be called as a context manager, so that you can temporarily change default parameters (this is used inexamples/repr_gallery.py
). (This is more general than this PR, but I used it extensively inrepr_gallery
).A detailed set of before and after views are attached as PDFs, generated by the new
examples/repr_gallery.ipynb
andexampes/repr_gallery.py
files:Some highlights (pulled from
repr_gallery
):