GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Update I/O system repr() and str() by murrayrm · Pull Request #1091 · python-control/python-control Conversation
|
|
coverage: 94.686% (-0.1%) from 94.781% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| escape_chars = { | ||
| '$': r'\$', | ||
| '<': '<', | ||
| '>': '>', |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| Notes | ||
| ----- | ||
| By default, the representation for an input/output is set to 'info'. |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be > 0?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # | ||
| reset_defaults() # start from a clean slate | ||
|
|
||
| # Verions 0.10.2 |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| # Verions 0.10.2 | |
| # Version 0.10.2 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| sys.repr_format = format | ||
| out = repr(sys) | ||
| if format == 'eval': | ||
| assert re.search(expected, out) != None |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@slivingston Thanks for the detailed review. Let me know if additional comments are coming, otherwise I'll go head and merge. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
b44d5ba to
0f0fad0
Compare
I will do one more look now and wrap it up within 2 hours. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.