Skip to content

Conversation

@bnavigator
Copy link
Collaborator

As requested by #86

Please review. Especially the variable names of input and output matrices and the index ranges mentioned in the docstrings.

Copy link
Collaborator

@roryyorke roryyorke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is as far as I can get for today.

@murrayrm, periodic Schur decompositions are entirely new to me; if you've got any background on this, could you take a look?

Returns
-------
HQ : ndarray
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think specifying the exact size of HQ would be good (I gather HQ.shape will be (n, n, p) ?

Similarly, could you say what the size of Tau will be? (this one I couldn't guess)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HQ.shape is the same shape as input A. Minimum is (max(n, 1), max(n, 1), p) but could be larger.
Tau.shape is (max(1, n-1), p)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it worth adding this to the docstring?

If n < min(A.shape[:2]), HQ would still have the same size as A? Would the n: rows and columns of HQ then be uninitialized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's the way many of the SLICOT routines do it. Adding this to the docstring and rebasing once more.

We have a bunch of different docstring styles for the various functions. Opening another issue for that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Do you agree with this, from #88 (comment):

 So should that be "upper diagonal (or Hessenberg) in [:ilo-1]" ?

Copy link
Collaborator Author

@bnavigator bnavigator Apr 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. General rule is:

  • Python-index = Fortran-index - 1
  • Python-slice-start = Fortran-slice-start-1
  • Python-slice-end:Fortran-slice-end

Or put differently:
A(x,y) = A[x-1,y-1]
A(a:b,c:d) = A[a-1:b,c-1:d]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix committed

@roryyorke
Copy link
Collaborator

Please let me know if 1a5d38f looks right to you; it's the same indexing change we discussed for mb03vd applied to mb03wd.

@bnavigator
Copy link
Collaborator Author

It looks right, but I have committed another one, also same issue as above with [ilo-1, ilo-2] ;)

@roryyorke
Copy link
Collaborator

roryyorke commented Apr 12, 2020

Edit: oops, I didn't merge on my local branch. You've fixed all this, thanks.

I missed that. Ah, I now see what your earlier comment about [ilo-1, ilo-2] was, sorry. So mb03wd also needs a fix.

Here's how things stand at 4d3a03d; I'm going to change mb03wd to match mb03vd.

  • mb03vd
    for: A_1(ILO,ILO-1) = 0
    py: A_1[ilo-1,ilo-2] = 0

  • mb03wd
    for: H_1(ILO,ILO-1) = 0
    py: H_1[ilo-1,ilo] H_1[ilo-1,ilo-2]

@roryyorke roryyorke merged commit 35743db into python-control:master Apr 12, 2020
@bnavigator bnavigator deleted the schur-wrappers branch December 31, 2020 13:48
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

Successfully merging this pull request may close these issues.

2 participants