-
Notifications
You must be signed in to change notification settings - Fork 438
Updated gram to support discrete-time systems #969
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
Conversation
I have fixed the failing FutureWarning test. Could you please rebase onto current main? |
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.
Thanks for working on these ancient TODO lines! They're 13 years old!
control/statefbk.py
Outdated
raise ValueError("Oops, the system is unstable!") | ||
|
||
else: | ||
raise ValueError("sys") |
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 imagine an error message from this line could be very confusing.
Is this even reachable by normal API usage? If so, could you please cover it in statefbk_test.py? If not, do we really need the line or is a if sys.isctime(): .. else: ...
construct enough?
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 is true. The only time this would ever get raised is if someone passed an object that returns false to both isctime
and isdtime
. I put it in because it's bad practice to do if cond1: ... elif cond2:
with no else but it would only realistically get raised during development if something was wrong. That's why I left the message very simple because no real user is likely to ever see it, hopefully!
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 sys.isdtime
literally returned not sys.isctime
then we can be sure this will never occur. But it looks more complicated than that.
Still, this might be a case of 'belt and braces' (belt and suspenders) and I'm happy to delete it.
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.
Okay, this may be pedantic but for the sake of coverage, please create a test so that the test suite at least hits that line once.
Sorry, how do I do that again? I just tried using these instructions and not sure it worked. |
Those instructions are not complete for your case. Your PR comes from basicmachines/main. That makes it a bit more complicated than our own instructions in https://github.com/python-control/python-control/wiki/How-to-contribute-with-a-pull-request You need to: option A
option B
(Why do you have two forks? Better work with properly named feature branches) |
Sorry I probably shouldn't have created another fork. The previous fork has all that work I did on the plot functions previously (I shared a link to it with Richard in case he needs to refer to it). It's all far behind the origin now. I'll fix that conditional statement so no test is needed and re-submit the PR. |
I committed the change so it should be ready to merge now. I only ran the statefbk_test.py tests and there are still two fails but I don't think these are anything to do with the changes I made:
|
That is an "Expected fail" and nothing to worry about here. |
You still need to rebase to current upstream/main (e1e33e4). The current diff (https://github.com/python-control/python-control/pull/969/files) and commit list (https://github.com/python-control/python-control/pull/969/commits) shows changes from Henrik's and my merged PRs which we do not want to duplicate.
|
@billtubbs if you're getting tied up doing a rebase (as has often happened to me) maybe easier would be to try working on a fresh branch from an up-to-date main and re-doing your changes by hand? |
Hi Ben, Sawyer, sorry I got distracted and forgot to finish this. Thanks for the reminder I will get back to it this week. |
@billtubbs I'm likely to release a new version of python-control in the next day or two. If you have a chance to rebase this PR, I'll include it in the v0.10.0 (otherwise v0.10.1). |
Sorry for delay. I just rebased it using your earlier instructions and it seemed to work. Running the tests again now... |
Tests seemed to work. Is this success?
|
Thanks @billtubbs. The test output looks fine. |
gram
function for observability/controllability Gramians did not support discrete time systems, as described in #967 (comment).Updated
control/statefbk.py
and tests incontrol/tests/statefbk_test.py
.