- Notifications
You must be signed in to change notification settings - Fork446
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJan 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
sawyerbfuller left a comment
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 systemsu0 ,u1 vsu[0] ,u[1] for other systems a bug or a feature? Maybe a bug that this PR uncovered.
murrayrm commentedJan 4, 2025
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). |
murrayrm left a comment
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 | ||
| ifmajor==0andminor<=10andpatch<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 useseval as default, so this may not be needed. We could setiosys.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
| def__str__(self): | ||
| returnf"{InputOutputSystem.__str__(self)}\n\n"+ \ | ||
| out=f"{InputOutputSystem.__str__(self)}" | ||
| iflen(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): | ||
| returnStateSpace.__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)
slivingston left a comment
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 someinterest | |
| # changes in representation formats, but also has someinteresting |
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
| # | ||
| 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
| deftest_nlsys_basic(): | ||
| defkincar_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) | ||
| ifformat=='eval': | ||
| assertre.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.
| assertre.search(expected,out)!=None | |
| assertre.search(expected,out)isnotNone |
Preferred style (inPEP 8) isis 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)) | ||
| forattrin ['A','B','C','D']: | ||
| assertgetattr(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.
| assertgetattr(sys,attr)==getattr(sys,attr) | |
| assertgetattr(sys,attr)==getattr(new,attr) |
murrayrm commentedJan 13, 2025
@slivingston Thanks for the detailed review. Let me know if additional comments are coming, otherwise I'll go head and merge. |
b44d5ba to0f0fad0Compareslivingston commentedJan 13, 2025
I will do one more look now and wrap it up within 2 hours. |
ec7dc8a intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 inLTIsystems to show the state space matrices, numerator/denominator, or freuquency response data. This format is similar to what was done before this PR forLTIclasses, 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 aformatparameter ('info', 'eval', or 'latex').display_formatparameter 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 topolyorzpkon system creation, that overrides the default.str()(print) representation ofInputOutputSystemsto includedtafter the state/input/output names (suppressed if it matches the defaultdt).str()(print) representation forNonlinearIOSystemsto include list of parameter labels.str()(print) representation forInterconnectedSystemsto 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.printoptionsto system arrays before generating strings, so you get consistent formatting across representations (including latex) and you can use things likesuppressto 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.ipynbandexampes/repr_gallery.pyfiles 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.defaultsoptions 'iosys.repr_format' and 'iosys.repr_show_count' to control representation formats.combine_tffunction 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.defaultsto 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.ipynbandexampes/repr_gallery.pyfiles:Some highlights (pulled from
repr_gallery):