Fix qr mode #2965

Merged
merged 3 commits into from Apr 4, 2013

Conversation

Projects
None yet
4 participants
@charris
Member

charris commented Feb 4, 2013

This PR addresses gh-2961 by adding a 'big' mode to the qr factorization modes so that true full factorizations can be obtained. The current 'full' mode results in what is commonly called a thin factorization. Because changing the meaning of 'full' and adding a 'thin' mode would be a nasty process, adding a new 'big' mode looked like the easiest way to go. If anyone has a better suggestion for the name of the new mode I'd be happy to hear it.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 4, 2013

Member

I think we ought to:

  • rename full to thin (but keep recognizing full as an alias for compatibility)
  • issue some sort of deprecation warning if someone does use full explicitly
  • eventually remove full

Then in the long run there are two options I guess? Either after full has been gone for a bit, reintroduce it as an alias for big, or else just leave it dead. In the former case we should use a FutureWarning when people specify full, and in the latter merely a DeprecationWarning.

Member

njsmith commented Feb 4, 2013

I think we ought to:

  • rename full to thin (but keep recognizing full as an alias for compatibility)
  • issue some sort of deprecation warning if someone does use full explicitly
  • eventually remove full

Then in the long run there are two options I guess? Either after full has been gone for a bit, reintroduce it as an alias for big, or else just leave it dead. In the former case we should use a FutureWarning when people specify full, and in the latter merely a DeprecationWarning.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 4, 2013

Member

Heh, that's what I had originally. We can preserve backward compatibility by keeping 'thin' the default and warning folks not to use 'full' at all as it is misleading, then maybe five years from now we can actually reuse 'full'.

Member

charris commented Feb 4, 2013

Heh, that's what I had originally. We can preserve backward compatibility by keeping 'thin' the default and warning folks not to use 'full' at all as it is misleading, then maybe five years from now we can actually reuse 'full'.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 4, 2013

Member

I've added a 'thin' option and now raise a FutureWarning when 'full' is used. We may want to revert the FutureWarning later, but it is here for discussion.

Member

charris commented Feb 4, 2013

I've added a 'thin' option and now raise a FutureWarning when 'full' is used. We may want to revert the FutureWarning later, but it is here for discussion.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 4, 2013

Member

The existing qr docs are quite confusing :-(. I think our economic means that we only fill in the upper-triangular part of the symmetric R matrix? B/c scipy's economic means something else... specifically, our full/thin. What a mess. Maybe scipy should make a similar change to rename their economic to thin? Maybe we should additionally rename our economic to something else, like r_upper?

Member

njsmith commented Feb 4, 2013

The existing qr docs are quite confusing :-(. I think our economic means that we only fill in the upper-triangular part of the symmetric R matrix? B/c scipy's economic means something else... specifically, our full/thin. What a mess. Maybe scipy should make a similar change to rename their economic to thin? Maybe we should additionally rename our economic to something else, like r_upper?

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 4, 2013

Member

The duplicated linalg functions in Scipy are a silly thing to have. (IMO, the versions in Numpy should be taken as the "canonical" ones and brought up to feature parity with the corresponding Scipy functions, after which the latter can be dropped.) But as long as we are stuck with them, it's a good idea to try to make the semantics in sync.

Member

pv commented Feb 4, 2013

The duplicated linalg functions in Scipy are a silly thing to have. (IMO, the versions in Numpy should be taken as the "canonical" ones and brought up to feature parity with the corresponding Scipy functions, after which the latter can be dropped.) But as long as we are stuck with them, it's a good idea to try to make the semantics in sync.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 4, 2013

Member

The return from the factorization has r in the upper triangular part and the Householder reflectors down the remainder of the columns. When mode='r' the triangular part is extracted and returned, when mode='economic' you get the whole thing. IIRC, there is also a 'tau' with scaling elements for the the reflectors. The only real use I could see for 'economic' in numpy would be for multiplying other matrices with q using the reflectors without going to multiplication with the full, possibly huge, q. The saving in time over 'r' is likely to be slight in normal use.

@pv Agree that it would be nice to rationalize the functions. Now we just need to decide what they should be ;)

Member

charris commented Feb 4, 2013

The return from the factorization has r in the upper triangular part and the Householder reflectors down the remainder of the columns. When mode='r' the triangular part is extracted and returned, when mode='economic' you get the whole thing. IIRC, there is also a 'tau' with scaling elements for the the reflectors. The only real use I could see for 'economic' in numpy would be for multiplying other matrices with q using the reflectors without going to multiplication with the full, possibly huge, q. The saving in time over 'r' is likely to be slight in normal use.

@pv Agree that it would be nice to rationalize the functions. Now we just need to decide what they should be ;)

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 4, 2013

Member

Maybe the upper triangular part + Householder reflectors thing should be
called "combined", since it stores both the Q and R information in a single
matrix?

I'm not sure how this relates to scipy.linalg.qr's "raw" either (which says
it returns (Q, TAU)). I guess Q is the same as numpy's "economic", and TAU
is some extra information needed to interpret the reflectors, but which
numpy doesn't make available?

On Mon, Feb 4, 2013 at 3:14 PM, Charles Harris notifications@github.comwrote:

The return from the factorization has r in the upper triangular part and
the Householder reflectors down the remainder of the columns. When mode='r'
the triangular part is extracted and returned, when mode='economic' you get
the whole thing. IIRC, there is also a 'tau' with scaling elements for the
the reflectors. The only real use I could see for 'economic' in numpy would
be for multiplying other matrices with q using the reflectors without going
to multiplication with the full, possibly huge, q. The saving in time over
'r' is likely to be slight in normal use.

@pv https://github.com/pv Agree that it would be nice to rationalize
the functions. Now we just need to decide what they should be ;)


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13105496.

Member

njsmith commented Feb 4, 2013

Maybe the upper triangular part + Householder reflectors thing should be
called "combined", since it stores both the Q and R information in a single
matrix?

I'm not sure how this relates to scipy.linalg.qr's "raw" either (which says
it returns (Q, TAU)). I guess Q is the same as numpy's "economic", and TAU
is some extra information needed to interpret the reflectors, but which
numpy doesn't make available?

On Mon, Feb 4, 2013 at 3:14 PM, Charles Harris notifications@github.comwrote:

The return from the factorization has r in the upper triangular part and
the Householder reflectors down the remainder of the columns. When mode='r'
the triangular part is extracted and returned, when mode='economic' you get
the whole thing. IIRC, there is also a 'tau' with scaling elements for the
the reflectors. The only real use I could see for 'economic' in numpy would
be for multiplying other matrices with q using the reflectors without going
to multiplication with the full, possibly huge, q. The saving in time over
'r' is likely to be slight in normal use.

@pv https://github.com/pv Agree that it would be nice to rationalize
the functions. Now we just need to decide what they should be ;)


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13105496.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 4, 2013

Member

If that's correct then another option would be to implement a "raw" mode
compatible with scipy, and deprecate numpy's "economic" entirely (not clear
anyone would care or notice), eventually removing it.

On Mon, Feb 4, 2013 at 3:19 PM, Nathaniel Smith njs@pobox.com wrote:

Maybe the upper triangular part + Householder reflectors thing should be
called "combined", since it stores both the Q and R information in a single
matrix?

I'm not sure how this relates to scipy.linalg.qr's "raw" either (which
says it returns (Q, TAU)). I guess Q is the same as numpy's "economic", and
TAU is some extra information needed to interpret the reflectors, but which
numpy doesn't make available?

On Mon, Feb 4, 2013 at 3:14 PM, Charles Harris notifications@github.comwrote:

The return from the factorization has r in the upper triangular part and
the Householder reflectors down the remainder of the columns. When mode='r'
the triangular part is extracted and returned, when mode='economic' you get
the whole thing. IIRC, there is also a 'tau' with scaling elements for the
the reflectors. The only real use I could see for 'economic' in numpy would
be for multiplying other matrices with q using the reflectors without going
to multiplication with the full, possibly huge, q. The saving in time over
'r' is likely to be slight in normal use.

@pv https://github.com/pv Agree that it would be nice to rationalize
the functions. Now we just need to decide what they should be ;)


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13105496.

Member

njsmith commented Feb 4, 2013

If that's correct then another option would be to implement a "raw" mode
compatible with scipy, and deprecate numpy's "economic" entirely (not clear
anyone would care or notice), eventually removing it.

On Mon, Feb 4, 2013 at 3:19 PM, Nathaniel Smith njs@pobox.com wrote:

Maybe the upper triangular part + Householder reflectors thing should be
called "combined", since it stores both the Q and R information in a single
matrix?

I'm not sure how this relates to scipy.linalg.qr's "raw" either (which
says it returns (Q, TAU)). I guess Q is the same as numpy's "economic", and
TAU is some extra information needed to interpret the reflectors, but which
numpy doesn't make available?

On Mon, Feb 4, 2013 at 3:14 PM, Charles Harris notifications@github.comwrote:

The return from the factorization has r in the upper triangular part and
the Householder reflectors down the remainder of the columns. When mode='r'
the triangular part is extracted and returned, when mode='economic' you get
the whole thing. IIRC, there is also a 'tau' with scaling elements for the
the reflectors. The only real use I could see for 'economic' in numpy would
be for multiplying other matrices with q using the reflectors without going
to multiplication with the full, possibly huge, q. The saving in time over
'r' is likely to be slight in normal use.

@pv https://github.com/pv Agree that it would be nice to rationalize
the functions. Now we just need to decide what they should be ;)


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13105496.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 4, 2013

Member

That's right about 'raw'. The tau can actually be computed from the diagonal elements, IIRC, it is the inverse, but lets you use multiplication instead of division and is expected in more general LAPACK routines. I think the 'raw' return has more future as a useful addition because at some point we might want to expose more LAPACK functions that are already in lapack_lite.c. The current 'economic' option is, I think, pretty useless since it can't be used for multiplication and the r probably needs to be extracted anyway.

Member

charris commented Feb 4, 2013

That's right about 'raw'. The tau can actually be computed from the diagonal elements, IIRC, it is the inverse, but lets you use multiplication instead of division and is expected in more general LAPACK routines. I think the 'raw' return has more future as a useful addition because at some point we might want to expose more LAPACK functions that are already in lapack_lite.c. The current 'economic' option is, I think, pretty useless since it can't be used for multiplication and the r probably needs to be extracted anyway.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 5, 2013

Member

Suggestion for the modes, give that the input matrix has dimensions (m, n) and k = min(m, n):

'thin' -- returns q, r with dimensions (m, k), (k, n) respectively.
'full' -- returns q, r with dimensions (m, m), (m, nI respectively.
'r' -- returns r with dimensions (k, n)
'qt' -- return the q, tau factors needed for multiplication, 'raw' in scipy

Alternatives to 'qt(au)' might be 'qf(actor)' or 'qh(ousholder)'. I'd suggest leaving the q part in Fortran contiguous form. The 'economic' mode would be deprecated and 'big' used for 'full' until such time as the switch can be made.

Member

charris commented Feb 5, 2013

Suggestion for the modes, give that the input matrix has dimensions (m, n) and k = min(m, n):

'thin' -- returns q, r with dimensions (m, k), (k, n) respectively.
'full' -- returns q, r with dimensions (m, m), (m, nI respectively.
'r' -- returns r with dimensions (k, n)
'qt' -- return the q, tau factors needed for multiplication, 'raw' in scipy

Alternatives to 'qt(au)' might be 'qf(actor)' or 'qh(ousholder)'. I'd suggest leaving the q part in Fortran contiguous form. The 'economic' mode would be deprecated and 'big' used for 'full' until such time as the switch can be made.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 5, 2013

Member

I like it, except, is 'qt' or 'qtau' enough better than 'raw' to justify
switching scipy around? (And for that matter, it isn't really q and tau,
right? it's q-and-r-together plus tau? I know LAPACK calls it Q but it
seems unnecessarily confusing to propagate that naming decision up to the
python level.)

On Mon, Feb 4, 2013 at 9:08 PM, Charles Harris notifications@github.comwrote:

Suggestion for the modes, give that the input matrix has dimensions (m, n)
and k = min(m, n):

'thin' -- returns q, r with dimensions (m, k), (k, n) respectively.
'full' -- returns q, r with dimensions (m, m), (m, nI respectively.
'r' -- returns r with dimensions (k, n)
'qt' -- return the q, tau factors needed for multiplication, 'raw' in scipy

Alternatives to 'qt(au)' might be 'qf(actor)' or 'qh(ousholder)'. I'd
suggest leaving the q part in Fortran contiguous form. The 'economic' mode
would be deprecated and 'big' used for 'full' until such time as the switch
can be made.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13114913.

Member

njsmith commented Feb 5, 2013

I like it, except, is 'qt' or 'qtau' enough better than 'raw' to justify
switching scipy around? (And for that matter, it isn't really q and tau,
right? it's q-and-r-together plus tau? I know LAPACK calls it Q but it
seems unnecessarily confusing to propagate that naming decision up to the
python level.)

On Mon, Feb 4, 2013 at 9:08 PM, Charles Harris notifications@github.comwrote:

Suggestion for the modes, give that the input matrix has dimensions (m, n)
and k = min(m, n):

'thin' -- returns q, r with dimensions (m, k), (k, n) respectively.
'full' -- returns q, r with dimensions (m, m), (m, nI respectively.
'r' -- returns r with dimensions (k, n)
'qt' -- return the q, tau factors needed for multiplication, 'raw' in scipy

Alternatives to 'qt(au)' might be 'qf(actor)' or 'qh(ousholder)'. I'd
suggest leaving the q part in Fortran contiguous form. The 'economic' mode
would be deprecated and 'big' used for 'full' until such time as the switch
can be made.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13114913.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 5, 2013

Member

I would have gone with 'raw' except that the function has always allowed using only the first letter of the mode and 'raw' would conflict with 'r'. I'm tending towards 'qrf' as it is the suffix in the LAPACK factorization routine. Thinking ahead to the LU factorization which is also in lapack_lite and should also be exposed IMHO, that would use 'trf' for the same sort of raw return.

Member

charris commented Feb 5, 2013

I would have gone with 'raw' except that the function has always allowed using only the first letter of the mode and 'raw' would conflict with 'r'. I'm tending towards 'qrf' as it is the suffix in the LAPACK factorization routine. Thinking ahead to the LU factorization which is also in lapack_lite and should also be exposed IMHO, that would use 'trf' for the same sort of raw return.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Feb 5, 2013

Member

I guess these are all fine, though I'll just say... frankly I'm not too
impressed by the whole "first letter suffices" rule anyway. It's sort of
one of those classic dumb trade-offs where you save three keystrokes
writing the thing (so like, 0.5 seconds maybe?), and then every time you
read the code you have to check the docs to remember what the heck this
'mode="f"' thing means. If you want perl you know where to find it, etc. So
I wouldn't cry if we deprecated that too in general, and just never
implemented it for the new mode names...

On Tue, Feb 5, 2013 at 9:07 AM, Charles Harris notifications@github.comwrote:

I would have gone with 'raw' except that the function has always allowed
using only the first letter of the mode and 'raw' would conflict with 'r'.
I'm tending towards 'qrf' as it is the suffix in the LAPACK factorization
routine. Thinking ahead to the LU factorization which is also in
lapack_lite and should also be exposed IMHO, that would use 'trf' for the
same sort of raw return.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13139943.

Member

njsmith commented Feb 5, 2013

I guess these are all fine, though I'll just say... frankly I'm not too
impressed by the whole "first letter suffices" rule anyway. It's sort of
one of those classic dumb trade-offs where you save three keystrokes
writing the thing (so like, 0.5 seconds maybe?), and then every time you
read the code you have to check the docs to remember what the heck this
'mode="f"' thing means. If you want perl you know where to find it, etc. So
I wouldn't cry if we deprecated that too in general, and just never
implemented it for the new mode names...

On Tue, Feb 5, 2013 at 9:07 AM, Charles Harris notifications@github.comwrote:

I would have gone with 'raw' except that the function has always allowed
using only the first letter of the mode and 'raw' would conflict with 'r'.
I'm tending towards 'qrf' as it is the suffix in the LAPACK factorization
routine. Thinking ahead to the LU factorization which is also in
lapack_lite and should also be exposed IMHO, that would use 'trf' for the
same sort of raw return.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2965#issuecomment-13139943.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 5, 2013

Member

Can't say I disagree about using the first letter. The list of potential modifications is getting long, I'm going to send a post to the list for discussion.

Member

charris commented Feb 5, 2013

Can't say I disagree about using the first letter. The list of potential modifications is getting long, I'm going to send a post to the list for discussion.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Feb 6, 2013

Contributor

@charris Thanks for sending this to the list. I think the suggested thin/big/fat/economic/r options are very confusing, especially since my first thought was that they would call different algorithms like the tall-skinny qr.

If it were up to me I'd prefer:

  1. complete: Q is a M-by-M matrix, i.e. a complete orthogonal basis. (Alternatives: square, whole, all, total.)
  2. reduced: Q is a M-by-K matrix. (Alternatives: thin)
  3. r: Only return R.

The current economic mode would make more sense to me if it was instead called raw, but I don't use it personally.

Contributor

bfroehle commented Feb 6, 2013

@charris Thanks for sending this to the list. I think the suggested thin/big/fat/economic/r options are very confusing, especially since my first thought was that they would call different algorithms like the tall-skinny qr.

If it were up to me I'd prefer:

  1. complete: Q is a M-by-M matrix, i.e. a complete orthogonal basis. (Alternatives: square, whole, all, total.)
  2. reduced: Q is a M-by-K matrix. (Alternatives: thin)
  3. r: Only return R.

The current economic mode would make more sense to me if it was instead called raw, but I don't use it personally.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 6, 2013

Member

@bfroehle Those look like good choices. If we require that the mode be spelled out for the new modes that will avoid conflicts with the traditional only-need-first-letter modes, which can then be deprecated. 'economic' isn't quite the same as 'raw', it doesn't contain the scaling factors tau which won't be needed until we expose some of the multiplication routines, but I'd like to have them in place.

Member

charris commented Feb 6, 2013

@bfroehle Those look like good choices. If we require that the mode be spelled out for the new modes that will avoid conflicts with the traditional only-need-first-letter modes, which can then be deprecated. 'economic' isn't quite the same as 'raw', it doesn't contain the scaling factors tau which won't be needed until we expose some of the multiplication routines, but I'd like to have them in place.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Feb 6, 2013

Member

@bfroehle, I've used your suggestions.

This PR should not be committed as is, I will squash the commits and push it separately when it is accepted.

Member

charris commented Feb 6, 2013

@bfroehle, I've used your suggestions.

This PR should not be committed as is, I will squash the commits and push it separately when it is accepted.

charris added some commits Apr 2, 2013

ENH: Add `raw`, `reduced`, `complete` modes to qr factorization.
If K = min(M, N) where the matrix to be factored has dimensions MxN,
then

'reduced'  : returns q, r with dimensions (M, K), (K, N) (default)
'complete' : returns q, r with dimensions (M, M), (M, N)
'r'        : returns r only with dimensions (K, N)
'raw'      : returns h, tau with dimensions (N, M), (K,)
'full'     : alias of 'reduced', deprecated
'economic' : returns h from 'raw', deprecated.

The options 'reduced', 'complete, and 'raw' are new. The default is
'reduced' and to maintain backward compatibility with earlier versions
of numpy both it and the old default 'full' can be omitted. Note that
array `h` returned in 'raw' mode is transposed for calling Fortran. Both
the 'full' and 'economic' modes are deprecated. For backwards
compatibility the modes 'full', 'economic' may be passed using only the
first letter but all others must be spelled out.
TST: Add more tests for qr factorization.
The new tests cover the new modes 'complete' and 'raw'. The testing of
the 'reduced', aka 'full' mode is improved and tests are added for the
deprecation of the 'full' and 'economic' modes. A new file
`numpy/linalg/tests/test_deprecations.py` was added for the deprecation
tests.
@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Apr 2, 2013

Member

Squashed and documentation added to the 1.8 release notes.

Member

charris commented Apr 2, 2013

Squashed and documentation added to the 1.8 release notes.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Apr 2, 2013

Member

I've lost track of this... how does what we've ended up with compare to scipy, and do we need to coordinate with them?

Member

njsmith commented Apr 2, 2013

I've lost track of this... how does what we've ended up with compare to scipy, and do we need to coordinate with them?

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Apr 2, 2013

Member

The qr in scipy has modes {'full', 'r', 'economic', 'raw'}, but only 'r' and 'raw' have the same meanings as in numpy. The scipy 'full' is what I have added as 'complete' and scipy 'economic' is the new numpy 'reduced', nee 'full'. The defaults in both is 'full', but the results differ ;) So I think the new names provide a way to bring the two into agreement, but they aren't there yet.

@pv What do you think about these choices?

Member

charris commented Apr 2, 2013

The qr in scipy has modes {'full', 'r', 'economic', 'raw'}, but only 'r' and 'raw' have the same meanings as in numpy. The scipy 'full' is what I have added as 'complete' and scipy 'economic' is the new numpy 'reduced', nee 'full'. The defaults in both is 'full', but the results differ ;) So I think the new names provide a way to bring the two into agreement, but they aren't there yet.

@pv What do you think about these choices?

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Apr 4, 2013

Member

No feedback here, so I'm going to put this in.

Member

charris commented Apr 4, 2013

No feedback here, so I'm going to put this in.

charris added a commit that referenced this pull request Apr 4, 2013

@charris charris merged commit d310678 into numpy:master Apr 4, 2013

1 check passed

default The Travis build passed
Details

@charris charris deleted the charris:fix-qr-mode branch Apr 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment