Skip to content

Add slicing access for state-space models with tests #1012

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 27, 2024
Merged

Add slicing access for state-space models with tests #1012

merged 4 commits into from
Jun 27, 2024

Conversation

guptavaibhav0
Copy link
Contributor

This PR adds the capability to access state-space models using splice. This should help fix #256.

Current Issue:

>>> b =  ct.ss([[2, 3], [3, 4]], [[2, 1], [2, 3]], [[2, 4]], [[1, 2]], 0.1)
>>> b[:2, 1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../.venv/lib/python3.12/site-packages/control/statesp.py", line 1224, in __getitem__
    self.A, self.B[:, inpdx], self.C[outdx, :], self.D[outdx, inpdx],
            ~~~~~~^^^^^^^^^^
IndexError: only integers, slices (`:`), ellipsis (`...`), numpy.newaxis (`None`) and integer or boolean arrays are valid indices

Changes in this PR:

  • Adds a small check for indexing of transfer functions.
  • Adds slicing in the state-space model with proper tests.
>>> b =  ct.ss([[2, 3], [3, 4]], [[2, 1], [2, 3]], [[2, 4]], [[1, 2]], 0.1)
>>> print(b[:2, 1])
<StateSpace>: sys[0]$indexed
Inputs (1): ['u[1]']
Outputs (1): ['y[0]']
States (2): ['x[0]', 'x[1]']

A = [[2. 3.]
     [3. 4.]]

B = [[1.]
     [3.]]

C = [[2. 4.]]

D = [[2.]]

dt = 0.1

@coveralls
Copy link

Coverage Status

coverage: 94.473% (-0.02%) from 94.49%
when pulling 0f08b1e on guptavaibhav0:main
into dbc998d on python-control:main.

@murrayrm
Copy link
Member

@ guptavaibhav0 Please rebase this on the last main branch to allow the CI tests to run. Please note that the failure of "Conda-based pytest / Py3.10; conda Slycot; no Pandas; no CVXOPT" looks like something unrelated and should clear up when the tests are re-run.

@guptavaibhav0
Copy link
Contributor Author

@murrayrm I have done the rebase and hopefully, this should fix it!

@coveralls
Copy link

Coverage Status

coverage: 94.505% (-0.02%) from 94.522%
when pulling 6b61ed0 on guptavaibhav0:main
into ad6b49e on python-control:main.

murrayrm
murrayrm reviewed Jun 26, 2024
View reviewed changes
control/xferfcn.py Outdated
@@ -758,7 +760,12 @@ def __pow__(self, other):
return (TransferFunction([1], [1]) / self) * (self**(other + 1))

def __getitem__(self, key):
if not isinstance(key, Iterable) or len(key) != 2:
raise IOError('must provide indices of length 2 for state space')
Copy link
Member

Choose a reason for hiding this comment

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

Change "state space" to "transfer functions".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated in the latest commit.

control/tests/statesp_test.py Outdated
Comment on lines 477 to 480
[(0, 1),
(slice(0, 1, 1), 1),
(0, slice(1, 2, 1)),
(slice(0, 1, 1), slice(1, 2, 1))])
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add some more general tests. For example, how about [:2, 1] or [::-1, ::2]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some more general tests in the new commit. I also found a bug in the StateSpace init function which I have corrected. The init function doesn't check if the dimensions of the D matrix are correct if the input/output names are given.

control/statesp.py Outdated
Comment on lines 1226 to 1228
self.dt, name=sysname,
inputs=[self.input_labels[i] for i in list(inpdx)],
outputs=[self.output_labels[i] for i in list(outdx)])
self.dt, name=sysname, inputs=self.input_labels[inpdx], outputs=self.output_labels[outdx])
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason for this change? It looks like the updated final line goes past column 79, which we try not to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change was needed since slice is not iterable. I have pushed a new commit with a maximum column length of 79.

@coveralls
Copy link

Coverage Status

coverage: 94.506% (-0.02%) from 94.522%
when pulling a0fc6bc on guptavaibhav0:main
into ad6b49e on python-control:main.

@murrayrm murrayrm merged commit e5394c4 into python-control:main Jun 27, 2024
23 checks passed
@murrayrm murrayrm added this to the 0.10.1 milestone Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murrayrm murrayrm murrayrm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.1
Development

Successfully merging this pull request may close these issues.

Create functions for better indexing of MIMO transfer functions
3 participants
@guptavaibhav0 @coveralls @murrayrm