Skip to content
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

Bug in TD04AD when ROWCOL='C' #6

Closed
roryyorke opened this issue Jan 11, 2017 · 4 comments
Closed

Bug in TD04AD when ROWCOL='C' #6

roryyorke opened this issue Jan 11, 2017 · 4 comments
Assignees

Comments

@roryyorke
Copy link
Collaborator

The attached Fortran, when compiled under Ubuntu 14.04, demonstrates the problem. Unpack the zip file, run make, and you should see

./tfm2ss
 INFO (should be 0):
 0
 NR (should be 0):
 0
 D (should be 64):
  64.
Parameter 5 to routine TB01XD� was incorrect

For M=P=1, the 'R' and 'C' cases are essentially the same thing, but the 'R' case works, while the 'C' case bombs out.

I think the bug is here in TD04AD.f. According to the docs for the just-called TB01PD, the returned IWORK is valid for the first N non-zero entries, but this code seems to ignore that. When the transformed system is static, N is likely to be 0, and at least in the cases I tested IWORK(1) = IWORK(2) = 1, so TB01XD gets invalid arguments.

I don't know how important this is; for the purposes of python-control, if we ever did want to use column-factored denominators, we could work with the transpose of G(s), transform that to a statespace system using the now row-factored denominators, and then transpose the result. TD04AD promises an upper block Hessenberg A matrix, so they need extra transformations for the column-factored case.

I suppose this all seems a bit arcane. I found this while working on #5, and I thought I might as well report it.

tfm2ss.zip

@slivingston
Copy link
Member

Thanks for this report. I think that it is worth fixing or, at least, adding some detection and notification in the case that is known to lead to failure.

@slivingston slivingston self-assigned this Jan 20, 2017
@murrayrm
Copy link
Member

@repagh Was this fixed in PR #27?

@repagh
Copy link
Member

repagh commented Dec 8, 2018

Yes, this is fixed

@roryyorke
Copy link
Collaborator Author

Confirm fix; the test case (see tfm2ss.zip in original report) now outputs

./tfm2ss
 INFO (should be 0):
           0
 NR (should be 0):
           0
 D (should be 64):
   64.000000000000000     
 INFO (should be 0):
           0
 NR (should be 0):
           0
 D (should be 64):
   64.000000000000000     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants