LQPackedQ postmultiplication fixup #23813
Conversation
Sacha0
added
bugfix
| @test Ac_mul_B(C, implicitQ) ≈ Ac_mul_B(zeroextCdown, explicitQ) | ||
| @test Ac_mul_Bc(C, implicitQ) ≈ Ac_mul_Bc(zeroextCdown, explicitQ) |
There was a problem hiding this comment.
This only deletes tests – are there any tests that could be added that would have caught this?
There was a problem hiding this comment.
Excellent point, and yes! :) I've added @test_throws making certain those shape combinations now yield DimensionMismatchs. Thanks!
base/linalg/lq.jl
Outdated
| # | ||
| # (2) the inner dimension in the multiplication is the LQPackedQ's first dimension. | ||
| # in this case, the LQPackedQ behaves like either its square/full form or its | ||
| # thin form depending on the shape of the other object in the multiplication. |
There was a problem hiding this comment.
Maybe it should be pointed out that thin actually means wide in the case of LQ`.
There was a problem hiding this comment.
Why?!? Can it be renamed?
There was a problem hiding this comment.
Agreed. Though the "thin" decomposition/factorization terminology inherited from QR seems the most widespread, I have also seen "reduced" and "truncated" decomposition/factorization, both of which seem like better choices when applied to orthogonal decompositions/factorizations generally (and particularly "truncated" for its descriptiveness). Perhaps we should select one of the latter terms (and an equivalent term for the "full" or "complete" decomposition/factorization) and use that term consistently for all orthogonal decompositions/factorizations? If so, perhaps opening a separate issue for that change would be best? Best!
There was a problem hiding this comment.
Why?!? Can it be renamed?
The LQ is simply QR transposed so when Q is thin in QR it is actually wide in LQ. There are two cases here: the comment and the keyword. Here I just thought of extending the comment to point out that the matrix is wide. For the keyword, notice that you only use when materializing a dense Q so it is not really relevant when you just use the factorization. However, it is a bit unfortunate that thin means wide in lq. Maybe we should negate the argument and have a square keyword instead.
There was a problem hiding this comment.
Considerations for future work in any case? :)
There was a problem hiding this comment.
I extended the docstring with a note briefly describing the somewhat confusing "thin"/"tall", "wide"/"short", "truncated"/"reduced" situation, and revised the language in the code comments to use the hopefully less confusing "truncated" and "square". Thoughts? Thanks!
There was a problem hiding this comment.
Should we just use "truncated" consistently?
There was a problem hiding this comment.
#23813 (comment) :). That change sounds like work for a future pull request though. Best!
There was a problem hiding this comment.
I agree. We should consider changing the language here but in a separate PR.
There was a problem hiding this comment.
Tracking issue: https://github.com/JuliaLang/julia/issues/23882
|
Thanks all! :) |
In #23803 I incorrectly extended
LQPackedQ's context-dependent multiplication behavior to some methods that should not have that behavior. This pull request makesLQPackedQ's postmultiplication behavior context-dependent only where necessary, and adds additional commentary describing the intended behavior. Best!