mat: calculate Q lazily when calling QR.ToQ #1970
Conversation
kortschak
force-pushed
the
mat/qr_very_tall
branch
from
96b76f7
to
6d26c5d
Compare
It's in the table (well, it will be when I remember to commit it). |
When a matrix is very tall, calculating Q will currently allocate a large Q at the end of the factorisation, even if it is not going to be used. The eager calculation was intended to prevent repeated re-calculation of Q when it is used. So move the Q calculation to ToQ, but make it conditional on the stored Q value being empty, and empty Q at the end of the factorisation.
kortschak
force-pushed
the
mat/qr_very_tall
branch
from
6d26c5d
to
2ad11ca
Compare
| @@ -98,23 +98,9 @@ func (qr *QR) factorize(a Matrix, norm lapack.MatrixNorm) { | |||
| lapack64.Geqrf(qr.qr.mat, qr.tau, work, len(work)) | |||
| putFloat64s(work) | |||
| qr.updateCond(norm) | |||
| qr.updateQ() | |||
There was a problem hiding this comment.
I'm afraid this doesn't work because q is currently needed in the At method. The test passes just because QTo is called in the test before EqualApprox (which calls At).
We could reconstruct the necessary row of Q in each call to At which is terrible but hopefully nobody uses QR as Matrix except in the call to Solve?
There was a problem hiding this comment.
The other alternative is to lazily calculate Q for At the same way it is for ToQ, with a warning that it may cause OoM. I think your approach is probably better. A warning in the docs that it will be extremely inefficient should be enough.
There was a problem hiding this comment.
We could lazily compute q in the At() method if q is nil or empty, in the same way it is done in the Qto.
But I guess I'm also a bit confused as to why we need q at all in At() in the first place. When we factorize a matrix a, qr stores a copy of that matrix. Why not return directly qr.At()?
Isn't https://github.com/gonum/gonum/blame/2ad11cabb395b96efc5b67fa1b64480762d9e703/mat/qr.go#L46 also faulty?
I'm expected we should return Q*R at element (i,j), but instead it seems we're returning Q*A at element (i,j), with A = Q*R, the matrix we factorize.
There was a problem hiding this comment.
The other alternative is to lazily calculate Q for At the same way it is for ToQ.
Agreed. If one element is needed, then most likely all are.
qr stores a copy of that matrix. Why not return directly qr.At()?
That copy is overwritten by lapack64.Geqrf which efficiently stores both Q and R in the same matrix.
There was a problem hiding this comment.
Another alternative is to indeed store a in Factorize and compute Q in QTo without storing it.
There was a problem hiding this comment.
I think we would need to copy a in this situation.
There was a problem hiding this comment.
In the end I think it's best to lazily compute Q in At. QR.At should never be called in practice.
When a matrix is very tall, calculating Q will currently allocate a large Q at the end of the factorisation, even if it is not going to be used. The eager calculation was intended to prevent repeated re-calculation of Q when it is used. So move the Q calculation to ToQ, but make it conditional on the stored Q value being empty, and empty Q at the end of the factorisation.
Please take a look.
Fixes #1968