- Notifications
You must be signed in to change notification settings - Fork441
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.
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 commentedJun 18, 2024
@ 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. |
@murrayrm I have done the rebase and hopefully, this should fix it! |
coveralls commentedJun 26, 2024
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') | |||
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.
Change "state space" to "transfer functions".
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 have updated in the latest commit.
control/tests/statesp_test.py Outdated
| [(0, 1), | ||
| (slice(0, 1, 1), 1), | ||
| (0, slice(1, 2, 1)), | ||
| (slice(0, 1, 1), slice(1, 2, 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.
It would be nice to add some more general tests. For example, how about[:2, 1] or[::-1, ::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.
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
| 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]) |
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.
Was there a reason for this change? It looks like the updated final line goes past column 79, which we try not to do.
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 change was needed sinceslice is not iterable. I have pushed a new commit with a maximum column length of 79.
coveralls commentedJun 27, 2024
e5394c4 intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
This PR adds the capability to access state-space models using splice. This should helpfix#256.
Current Issue:
Changes in this PR: