A14b: quick-wins batch 2 (#168 J zero-fill, #124 materialize guard) - #171
Merged
Conversation
LAPACK's geqp3 reads jpvt on entry: a nonzero entry marks that column as fixed and moves it to the front of the permutation. Neither CQRRPT driver zeroed the caller's J, so the buffer's prior contents silently steered the pivoting. In-tree callers pass a value-initialized std::vector and satisfy the precondition by accident, which is why every existing test stayed green; a binding layer handing over an uninitialized buffer saw the detected rank wander between runs at a fixed RNG state. Fixed in rl_cqrrpt.hh and in rl_cqrrpt_gpu.hh, which carries the identical line (its QRCP runs on the host). BQRRP was already safe, but via the std::fill at rl_bqrrp.hh:327 rather than its std::iota; corrected the comment there, which called that fill 'may not be necessary' when it is load-bearing for this same reason. Regression test in each language of the pair: a rank-deficient input (m=2000, n=50, true rank 40) factored three times at one RNG state, with J clean and then dirtied two different ways. Detected rank must be identical across trials. Fails on main (rank 40 then 41), passes here.
…#124) The generic materialize path forms buf = A * I, allocating an n-by-n identity behind the caller's back. Nothing bounded it, so materializing a wide operator could request an enormous hidden allocation with no diagnostic. Guard on a named constant (MATERIALIZE_IDENTITY_MAX_DIM = 16384, which is 2 GiB in double) using the randlapack_require idiom already used in this file, and point the error text at the type-specific overloads. Those overloads (DenseLinOp, SparseLinOp, CompositeOperator) do not form an identity and are deliberately left uncapped; they are the answer for operators too large for the fallback. Riley's original objection on this issue, that linop correctness tests should not prove an operator by applying it to the identity, is already addressed by those overloads existing.
…romised The cap message and the test-utils docstring both pointed callers at a CompositeOperator overload that did not exist, so a composite wider than the cap had no direct path. The overload materializes each operand through its own dispatch and forms the product with one gemm: no identity, no cap, and no call through the composite's operator(), which also makes it safe for correctness tests of operator() itself. Docstrings updated to match.
Comments now state only the technical constraint in a few lines; the provenance and full rationale live in the PR body and commit history.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Second quick-wins batch from the A14 issue-backlog pass, following PR #148.
Closes #168, closes #124.
Summary
#168 is the silent-wrong-answer one: geqp3 reads jpvt on entry, and neither CQRRPT driver zeroed the caller's J. The audit found the GPU driver carries the identical defect, which the issue did not mention. BQRRP was already safe, but through a fill whose comment disclaimed it as probably unnecessary; that comment is corrected.
#124 is addressed in two parts. The generic materialize fallback still forms A * I, but the hidden n-by-n identity allocation is now capped (16384 columns, 2 GiB in double) and fails loudly past that. The type-specific path Riley recommended now covers all three concrete operator types: DenseLinOp and SparseLinOp already materialized by direct copy, and this PR adds the missing CompositeOperator overload, which materializes each operand through its own dispatch and forms the product with one gemm. That path builds no identity, carries no cap, and never routes through the composite's operator(), so it is also valid for correctness tests of operator() itself.
Changes
rl_cqrrpt.hh,rl_cqrrpt_gpu.hh: zero the caller's J before geqp3 (CQRRPT silently depends on J being zero-initialized (geqp3 reads jpvt on entry) #168).rl_bqrrp.hh: correct the comment on the (already safe) J fill.rl_materialize.hh: cap the identity allocation in the generic fallback; add the CompositeOperator overload (Operators materialization caution #124).rl_test_utils.hh: materialize_linop docstring rewritten to describe the actual dispatch and where the circularity caution still applies.test/drivers/test_cqrrpt.cc,test_cqrrpt_gpu.cu: regression tests running the same rank-deficient factorization with clean and dirtied J buffers, CPU and GPU.test/linops/test_linops.cc: guard test for the cap; composite-vs-reference product test; wide-composite test proving the overload is exempt from the cap.