- Notifications
You must be signed in to change notification settings - Fork441
Complex matrices#376
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
Complex matrices#376
Uh oh!
There was an error while loading.Please reload this page.
Conversation
control/tests/statesp_test.py Outdated
| def test_evalfr_complex(self): | ||
| """Evaluate the frequency response at one frequency.""" | ||
| resp = [[4.6799374736968655 -34.9854626345217383j, |
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.
Please be easier on the precision of your reference values. This will almost certainly fail on some build systems due to rounding errors or depending on the used BLAS/LAPACK implementation.
https://travis-ci.org/python-control/python-control/jobs/656662515#L1598-L1631 |
What happens if you call a function that uses slycot and give it a |
lytex commentedMar 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've run Regarding the second warning, there is a complex routine |
lytex commentedMar 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've made a more detailed traceback of the warnings: Tests failing with |
If we compare sys.A against A we get a IndexError (not sure why), butbuilding a new auxiliary StateSpace seems to do the trick
If we compare sys.A against A we get a IndexError (not sure why), butbuilding a new auxiliary StateSpace seems to do the trick
lytex commentedMar 31, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Well, it seems that the currentf2py interface doesn't support complex arrays as an argument. I've tried to use the complex routine |
So you wrote a wrapper for |
According to thef2py typespec documentation, f2py signature files understand |
Yes f2py does support I'll do the pull request in a minute |
Will the CI check a new function that it is just added or there is some config file to declare with functions will be teseted? |
You need to write unit tests associated with any new functions that you add, otherwise they won't be checked. If you are adding something to |
I've changed The tests affected by this change should be |
The PR here in python-control will only be able to access the new function in Slycot when your PR over there is merged, a new version is released and the CI in python-control/master is updated to use this new release. So please first amendpython-control/Slycot#96 with a unit test, convince the maintainers to merge and wait for a release. The python-control unit tests which are run within Slycot's CI are also from master. Of course you could also modify the CI scripts here and over at Slycot#96 to use your respective modified branches for development, but that won't be suitable for merging. Thus I recommend to develop first in Slycot and as soon as that is done continue in python-control. |
|
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.
So with the new Slycot out, I do not see a reason why python-control should not allow complex arrays. Through the "ComplexWarning", the user is informed that a function discarded the imaginary part and is thus not suitable for complex input (yet).
| arr = np.matrix(data, dtype=float) | ||
| if np.isrealobj(data): | ||
| arr = np.matrix(data, dtype=float) | ||
| elif np.iscomplexobj(data): | ||
| arr = np.matrix(data, dtype=complex) | ||
| else: | ||
| arr = np.array(data, dtype=float) | ||
| if np.isrealobj(data): | ||
| arr = np.array(data, dtype=float) | ||
| elif np.iscomplexobj(data): | ||
| arr = np.array(data, dtype=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.
Is this really necessary? Why not just usematrix() andasarray() withoutdtype and figure out thedtype later?
bnavigatorJul 29, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Turns out if dtype is omitted completely, it could still beint and trip operations later on.
The test suite already has complex casting warnings from Scipy and Slycot functions returning complex arrays (with 0 imaginary part), which are passed to_ssmatrix. Thus, my approach in#438 is to usedtype=np.result_type(np.float, arr), which ensures to cast from int to at least float and retains complex if arr is already complex.
It is not necessary to enforce float and can raiseComplexWarning during casting; cf.python-control#376
Now I've written new tests that use complex matrices without touching the old ones