-
Notifications
You must be signed in to change notification settings - Fork 441
Allow signal names to be used for time/freq responses and subsystem indexing #1069
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
Allow signal names to be used for time/freq responses and subsystem indexing #1069
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.
Almost done with review; it will be done tomorrow. For now, I am sending minor style comments.
control/iosys.py
Outdated
|
||
def __array_finalize__(self, obj): | ||
# See https://numpy.org/doc/stable/user/basics.subclassing.html | ||
if obj is None: return |
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.
if obj is None: return | |
if obj is None: | |
return |
return
should go on the next line to make it more obvious what is in the if-block. Also, this is consistent with PEP 8:
Compound statements (multiple statements on the same line) are generally discouraged
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 example in the NumPy docs has obj is None: return
(on 1 line), but my style comment remains. I do not know why the example there has this 1-line style, but I suspect it is to keep examples short, which would also justify there being only 1 blank line after import
in those examples.
control/iosys.py
Outdated
idx = idx[0] | ||
|
||
# Convert int to slice so that numpy doesn't drop dimension | ||
if isinstance(idx, int): idx = slice(idx, idx+1, 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.
if isinstance(idx, int): idx = slice(idx, idx+1, 1) | |
if isinstance(idx, int): | |
idx = slice(idx, idx+1, 1) |
Similar to my other comment, statements should be on separate lines for clarity of style (as per PEP 8).
control/iosys.py
Outdated
# | ||
def _process_subsys_index(idx, sys_labels, slice_to_list=False): | ||
if not isinstance(idx, (slice, list, int)): | ||
raise TypeError(f"system indices must be integers, slices, or lists") |
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 string does not need the f
prefix, but OK to keep it if you anticipate expressions being added to it later.
control/tests/lti_test.py
Outdated
sys = ct.rss(4, 3, 3) | ||
subsys = sys[key] | ||
|
||
# Construct the system to be 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.
# Construct the system to be test | |
# Construct the system to be tested |
control/timeresp.py
Outdated
|
||
response(tranpose=True).input | ||
|
||
See :meth:`TimeResponseData.__call__` for more information. | ||
See :meth:`TimeResponseData.__call__` for more information. |
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.
See :meth:`TimeResponseData.__call__` for more information. | |
See :meth:`TimeResponseData.__call__` for more information. |
doc/conventions.rst
Outdated
@@ -242,6 +271,17 @@ such as the :func:`step_response` function applied to a MIMO system, | |||
which will compute the step response for each input/output pair. See | |||
:class:`TimeResponseData` for more details. | |||
|
|||
The input, output, and state elements of the response can be access using |
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 input, output, and state elements of the response can be access using | |
The input, output, and state elements of the response can be accessed using |
doc/plotting.rst
Outdated
Access to frequency response data is available via the attributes | ||
``omega``, ``magnitude``,` `phase``, and ``response``, where ``response`` | ||
represents the complex value of the frequency response at each frequency. | ||
The ``magnitude``,` `phase``, and ``response`` arrays can be indexed using |
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 ``magnitude``,` `phase``, and ``response`` arrays can be indexed using | |
The ``magnitude``, ``phase``, and ``response`` arrays can be indexed using |
doc/conventions.rst
Outdated
`control.config.defaults['iosys.indexed_system_name_prefix']` and | ||
`control.config.defaults['iosys.indexed_system_name_suffix']`. The default |
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.
`control.config.defaults['iosys.indexed_system_name_prefix']` and | |
`control.config.defaults['iosys.indexed_system_name_suffix']`. The default | |
``control.config.defaults['iosys.indexed_system_name_prefix']`` and | |
``control.config.defaults['iosys.indexed_system_name_suffix']``. The default |
rst files built with Sphinx require double backtick (``) to render as in-line code.
doc/conventions.rst
Outdated
Note: The `fresp` data member is stored as a NumPy array and cannot be | ||
accessed with signal names. Use `response.response` to access the complex |
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.
Note: The `fresp` data member is stored as a NumPy array and cannot be | |
accessed with signal names. Use `response.response` to access the complex | |
Note: The ``fresp`` data member is stored as a NumPy array and cannot be | |
accessed with signal names. Use ``response.response`` to access the complex |
doc/conventions.rst
Outdated
`response` object: `response.magnitude`, `response.phase`, and | ||
`response.response` (for the complex response). For MIMO systems, these |
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.
`response` object: `response.magnitude`, `response.phase`, and | |
`response.response` (for the complex response). For MIMO systems, these | |
``response`` object: ``response.magnitude``, ``response.phase``, and | |
``response.response`` (for the complex response). For MIMO systems, these |
This PR adds the ability to access response signals and subsystem inputs and outputs using signal names. As described in the documentation:
Time signal indexing:
Frequency response indexing:
Subsystem indexing:
Summary of changes:
NamedSignal
object, which is a subclass ofnp.ndarray
that overrides the__getitem__
method to allow processing of signal names via the_parse_key
method.iosys.NamedSignal._parse_key
and changes infrdata.py
andtimeresp.py
.StateSpace
,TransferFunction
, andFrequencyResponseData
systems. Seeiosys._process_subsys_index
and changes infrdata.py
,statesp.py
, andxferfcn.py
.