Uh oh!
There was an error while loading. Please reload this page.
Bugfix ab13bd - #200
Merged
Merged
Conversation
bnavigator
commented
Jul 20, 2023
Collaborator
Hi, thanks for your contribution. I didn't check in detail yet, but if it is really true that there is a call by reference, then this would be the case in many more functions than ab13bd and we have a general problem in the whole package. |
KybernetikJo
commented
Jul 20, 2023
ContributorAuthor
The following changes are actually enough: Intent(in,out,copy) copies the parameter. The function has no side effects on the state space matrices A,B,C,D TODO:
|
bnavigator
commented
Jul 20, 2023
Collaborator
Actually, according to https://numpy.org/doc/stable/f2py/signature-file.html, we should have the desired
I wonder if it the stray Slycot/slycot/src/analysis.pyf Line 324 in d844e7b causes the inputs to be altered? Could you check if you get altered inputs in other wrappers as well? |
bnavigator
commented
Jul 20, 2023
Collaborator
Yes, but we actually don't want the output anywhere. |
KybernetikJo
commented
Jul 20, 2023
ContributorAuthor
Thanks for your reply. I appreciate that. First, I am going to build up some theoretical understanding about f2py. Then I can run some experiments.
I am not sure. Why SLICOT returns them? |
roryyorke
commented
Jul 22, 2023
Collaborator
FWIW I can reproduce this with Slycot 0.5.2 on Ubuntu 20.04 in a Conda environment. From the SLICOT docs for AB13MD, the A, B, C and D arguments are input/output; they contain the state-space matrices of "the numerator factor Q of the right coprime factorization with inner denominator of G". So I think we violate this expectation of f2py:
Specifically, we violate "the function is expected to not change the value of this argument". Whoops! As @bnavigator noted, this could be a problem in quite a few places, we'll have to check. |
roryyorke
commented
Jul 22, 2023
Collaborator
See https://github.com/roryyorke/Slycot/tree/ab13md-alter-args, whence:
diff --git a/slycot/src/analysis.pyf b/slycot/src/analysis.pyf
index 633ff6ec..183a22f2 100644
--- a/slycot/src/analysis.pyf+++ b/slycot/src/analysis.pyf@@ -321,7 +321,6 @@ function ab13bd(dico,jobn,n,m,p,a,lda,b,ldb,c,ldc,d,ldd,nq,tol,dwork,ldwork,iwar
integer intent(hide),depend(n,m,p) :: ldwork = max(1,max(m*(n+m)+max(n*(n+5),max(m*(m+2),4*p)),n*(max(n,p)+4)+min(n,p)))
integer intent(out) :: iwarn
integer intent(out) :: info
- double precision intent(out) :: ab13bd
end function ab13bd
subroutine ab13dd(dico,jobe,equil,jobd,n,m,p,fpeak,a,lda,e,lde,b,ldb,c,ldc,d,ldd,gpeak,tol,iwork,dwork,ldwork,cwork,lcwork,info) ! in AB13DD.f
character intent(in) :: dico
diff --git a/slycot/src/analysis.pyf b/slycot/src/analysis.pyf
index 183a22f2..999e3b65 100644
--- a/slycot/src/analysis.pyf+++ b/slycot/src/analysis.pyf@@ -307,13 +307,13 @@ function ab13bd(dico,jobn,n,m,p,a,lda,b,ldb,c,ldc,d,ldd,nq,tol,dwork,ldwork,iwar
integer check(n>=0) :: n
integer check(n>=0) :: m
integer check(n>=0) :: p
- double precision dimension(n,n),depend(n) :: a+ double precision intent(in,out,copy),dimension(n,n),depend(n) :: a
integer intent(hide),depend(a) :: lda = shape(a,0)
- double precision dimension(n,m),depend(n,m) :: b+ double precision intent(in,out,copy),dimension(n,m),depend(n,m) :: b
integer intent(hide),depend(b) :: ldb = shape(b,0)
- double precision dimension(p,n),depend(n,p) :: c+ double precision intent(in,out,copy),dimension(p,n),depend(n,p) :: c
integer intent(hide),depend(c) :: ldc = shape(c,0)
- double precision dimension(p,m),depend(m,p) :: d+ double precision intent(in,out,copy),dimension(p,m),depend(m,p) :: d
integer intent(hide),depend(d) :: ldd = shape(d,0)
integer intent(out) :: nq
double precision :: tol |
roryyorke
commented
Jul 22, 2023
Collaborator
and 2625e1f, |
roryyorke
commented
Jul 22, 2023
Collaborator
I don't find the f2py docs all that clear. I don't understand the difference between intent variants For the record, |
roryyorke
commented
Jul 23, 2023
Collaborator
according to grep -n "dimension"~/src/slycot/slycot/src/*.pyf|grep -v "intent">> audit.out
grep -n "dimension"~/src/slycot/slycot/src/*.pyf|fgrep "intent(in)">> audit.outthere are 139 array arguments with implicit or explicit grep resultsSome of these are fine, e.g., the first arg to AB05MD is marked SLICOT as "input". We could wholesale change all of these to Worst-case is that some dependency is relying on this bad behaviour, but I think that is unlikely.
|
roryyorke
commented
Jul 23, 2023
Collaborator
I've compared each case from above with the comments in the associated SLICOT files, and marked each line OK or NOTOK. There are 19 "NOTOK" lines, including the 4 already found. I'd appreciate spot checks of the results. Next steps:
SLICOT-checked use of "intent(in)" arguments |
roryyorke
commented
Jul 23, 2023
Collaborator
I think there no problems at python-control level. Fix options:
Problem functionsSummary
ab13bdH2 / L2 norm ABCD passed straight through docstring does not indicate that arg will be changed not used in python-control Conclusions:
sb03odlyapunov equation problem SLICOT args: a, q if FACT is not 'F'. FACT can be specified by Advertised as "in-out" in doc string. used in python-control statefbk.gram. Args A and Q are constructed Conclusions:
sg03bdcholesky factor of generalized lyapunov equation solution problem args: a, e, q, z. Passed straight through to SLICOT. Not used in python-control Conclusions:
tc01od_rproblem args: pcoeff, qcoeff This is in,out for pcoeff and qcoeff are both mutated and returned. Conclusions
ab08ndmistake in initial analysis; args are input only in SLICOT tb05ad_nhused in slycot.tb05ad problem args a,b,c Complex freq. resp for A hessenberg (job='NH') A,B,C passed straight Ah - but only output if inita is "NG". For this case tb05ad_ng, with Conclusions:
|
ContributorAuthor
I think the line double precision intent(out) :: ab13bd is there because of ab13bd is a function, and not a subroutine. Fortran functions do have a return value, subrountines do not. I think ab13bd is the only function in SLICOT, I do not know why, but there might be a reason. |
ContributorAuthor
I was wrong. ab13cd is another function. There are a few of them in SLICOT. It also has a return value. AB13CD = ( GAMMAL + GAMMAU )/TWO |
KybernetikJo
commented
Jul 27, 2023
ContributorAuthor
FWIW, I have done some background reading about F2PY, and this is my current understanding. I am a newbie to fortran, therefore you should take this with a bit of toast. The stable version of f2py stable do have some duplication in the documentation. First, Fortran has two procedures, subroutines and functions. (This effects the F2PY signature file pyf) All arguments in Fortran77 can be seen as call by reference, by default. In Fortran90/95 the intent in Fortran90/95
In Fortran there is also the concept of a In Fortran2003 a |
ContributorAuthor
There are many option how to design F2PY =================== However, currently we need a solution for the case, where the parameter are labeled as
This option would potentially severely limit SLICOT procedures.
Here, we would have to deal with Slycot function side effects somewhere. Having said that, there are [].sort()I think in numpy there are not that common. For instance a_out=np.sort(a)
I still think this is the best option for us. This makes the slycot._wrapper function already more =================== We could use even more The output double precisionintent(out) :: ab13bdin the In the following example the functionab13bd(dico,jobn,n,m,p,a,lda,b,ldb,c,ldc,d,ldd,nq,tol,dwork,ldwork,iwarn,info) ! in AB13BD.f
character optional :: dico ='C'character optional :: jobn ='H'integer optional :: n =shape(a,0)
integer optional :: m =shape(b,1)
integer optional :: p =shape(c,0)
double precisiondimension(n,n), intent(in,out,copy) :: a
integer optional, depend(a) :: lda =shape(a,0)
double precisiondimension(n,m), intent(in,out,copy) :: b
integer optional, depend(b) :: ldb =shape(b,0)
double precisiondimension(p,n), intent(in,out,copy) :: c
integer optional, depend(c) :: ldc =shape(c,0)
double precisiondimension(p,m), intent(in,out,copy) :: d
integer optional, depend(d) :: ldd =shape(d,0)
integerintent(out) :: nq
double precision optional :: tol =0.0double precisionintent(hide,cache),dimension(ldwork),depend(ldwork) :: dwork
integer optional :: ldwork =max(1,max(m*(n+m)+max(n*(n+5),max(m*(m+2),4*p)),n*(max(n,p)+4)+min(n,p)))
integerintent(out) :: iwarn
integerintent(out) :: info
double precisionintent(out) :: ab13bd
endfunction ab13bdThis would allow a minimal function call but also some optional attributes are possible. |
ContributorAuthor
To sum it up, I would keep these commits and I would go for In the long run, we might reconsider the whole Good templates could be a good solution. |
KybernetikJo
marked this pull request as ready for review
July 27, 2023 20:42
ContributorAuthor
10 tasks
KybernetikJoforce-pushed
the
bugfix_ab13bd
branch
from
August 23, 2023 20:00
31924e7 to
823fda9Compare
This was referenced Aug 26, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Draft/PR fixes the issues #198 and #199.
change parameter check Bug, parameter check in ab13bd #198
uses a shallow copy of state space matrices Call by reference in ab13bd #199
Further testing is needed. Edge cases?
Unit tests (pytest) could be added?