P1673: LWG review 2023/11/06 (Kona)
Nobody has claimed this yet.
- Dominant language
- HTML
- Stars
- 29
- Forks
- 26
- PR merge metrics
- No merged PRs in 30d
Description
P1673: LWG review 2023/11/06 (Kona) and follow-ons
Resuming from [linalg.alg.blas2.trsv], where we stopped last time (2023/10/25).
Done, either in the main branch or in PR #426
- Generally, we need to qualify use of
forwardasstd::forward. Change throughout. - Replace
multiplyablewithmultipliablethroughout. (Fix spelling.) - Fix parallel algorithms "element access function" wording in [algorithms.parallel.defns] 3, by adding "or
mdspantypes" after "iterators": "All operations of the categories of the iterators ormdspantypes that the algorithm is instantiated with." - Add
BinaryDivideOpto the list in [algorithms.parallel.user] (Requirements on user-provided function objects). - Generally, say "
ymay aliasx" instead of "xandymay alias." This is consistent with the definition of "alias" in the front matter. Generally, "out-argument may alias in-argument." (PR #426) - Change label
xmmtoxxmm. (DONE) - When passing an ExecutionPolicy
execinto Effects-equivalent-to code, always usestd::forward<ExecutionPolicy>(exec). - Fix
matrix_rank_1_update*to use inout matrix as intended. Wording is wrong! - Change the updating wording to use $C'$ instead of relying on [linalg.general] 1.4. "Computes a matrix $C'$ such that $C' = C + \alpha A A^T$ and assigns each element of $C'$ to the corresponding element of $C$. Do this generally, e.g., for
triangular_matrix_vector_product. - Change
linalg.alg.blas3.*stable names tolinalg.algs.blas3.*. - Fix stable names for BLAS 2 rank-1 symmetric and Hermitian updates.
- For
symmetric_matrix_rank_k_updateandhermitian_matrix_rank_k_update, change the template parameter nameInMat1toInMat, because there's noInMat2. Fix this in the synopsis as well. - Fix everything under
transposed(e.g.,layout_transposed) to match wording of existing layouts. Remove references to the padded layouts. - Fix
layout_blas_packedanalogously. - Remove editorial comments and italics from [linalg.reqs.val].
- For [linalg.algs.reqs] Para 1, avoid ambiguity by changing "type requirements" to "Constraints." However, the first (1.1) is more than just a Constraint, so separate (1.1) to the top, and add the Constraint that ExecutionPolicy is an execution policy. (This indicates that the algorithm is a parallel algorithm.)
- Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?) (See LWG question below.)
Check with LWG
- For [linalg.algs.reqs] Para 1 (changing "type requirements" to "Constraints"; see above): Fixed by changing "type requirements" to "Constraints." Added the Constraint that ExecutionPolicy is an execution policy. Removed the requirement that the algorithms that take
ExecutionPolicyare parallel algorithms, because that would be circular with [algorithms.parallel] 2 ("A parallel algorithm is a function template listed in this document with a template parameter namedExecutionPolicy"). - For [mdspan.layout.policy.reqmts], it's not clear whether this says that
MP::mapping<E>must be valid for ANY specializationEofextents. That was not our intent. Should we add a Note or just add clarifying normative language? (The latter would probably not break existing user code, unless users are somehow depending on generic code being able to take any layoutMPand anyextentsspecializationE, and getMP::mapping<E>from it.) This matters for P1673 becauselayout_transposed::mapping<E>requires thatEhave rank 2.- Christian asked some LWG people; they said we might want to add a Note, but the wording itself permits this.
- Remove constraint that
stride(r)requires rank 2? Note thatstrideinlayout_leftandlayout_rightimpose a Constraint thatrank() > 0, butlayout_strideonly has this as a precondition. - Rename
layout_transposetolayout_transposed?
Resolved (don't need to check with LWG)
- Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?)
- We think [linalg.helpers.conj] partly covers this.
conj-if-needed(E)only evaluates toconj(E)ifconj(E)is a valid expression and the type ofEis not an arithmetic type. Otherwise, it evaluates toE.- If
conj(E)is a valid expression but "the function selected by overload resolution does not return the complex conjugate of its input, the program is ill-formed, no diagnostic required."
- The main question is whether
conj(E)is a linear algebra value type. - Ditto for the few algorithms that use
realandimag. - We don't need to ask this question.
- We think [linalg.helpers.conj] partly covers this.
Points for specific sections
[linalg.alg.blas2.trsv]
("take an x parameter" cannot be misread as "take a symbolic name of a parameter x"; "take a parameter x" can be misread in that way. Therefore, we prefer the existing wording, "take an x parameter.") (OK)
Para 6: "Computes a vector $x'$ such that..." (instead of just saying "Computes $x'$ ..."). (This permits nonunique $x'$.) (DONE)
Para 12: "Computes a vector $x'$ such that" (instead of just saying "Computes $x'$ ..."), as above. (DONE)
Para 13: Complexity method differs from in Para 7. Flip order of b and A in Para 13 to be consistent with Para 7. (DONE)
[linalg.alg.blas2.rank1]
Para 7: Put forward<ExecutionPolicy> around exec. Generally do this where needed. (DONE)
Split Para 7 into two sentences, to avoid possible confusion due to code-font semicolons. (DONE)
[linalg.alg.blas2.symrank1]
Para 3: Mandates here does not follow the decltype convention; change that. (DONE)
(LWG appreciates the Para 1 unambiguous self-reference : - ) . ) (OK)
Para 7: "Computes a matrix $A'$..." instead of "Computes $A'$...." (DONE)
Para 11: "Computes a matrix $A'$..." instead of "Computes $A'$...." (DONE)
[linalg.algs.blas2.rank2]
Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?) (DONE -- see question for LWG)
[linalg.algs.blas3.gemm]
Mandates: the above and also? (see notes)
Scribe missed some time
The in-place triangular_matrix_right_product etc. need a complexity clause. (DONE)
[linalg.alg.blas3.rankk]
Change label from linalg.alg to linalg.algs here. (DONE)
For symmetric_matrix_rank_k_update function signature, change InMat1 to InMat, because there's no InMat2. Fix this in the synopsis as well. (DONE)
Para 6: Change the updating wording to use $C'$ instead of relying on [linalg.general] 1.4. "Computes a matrix $C'$ such that $C' = C + \alpha A A^T$ and assigns each element of $C'$ to the corresponding element of $C$. Generally do that. It's OK to leave [linalg.general] 1.4, as it is still needed. (We'll need to fix in-place triangular_matrix_vector_product ("Computes $y = A y$"). (DONE)
For [linalg.algs.reqs] Para 1, does "type requirements" mean constraints? Yes, it does, to avoid ambiguity. Therefore, change "type requirements" to "Constraints." However, the first (1.1) is more than just a Constraint. We can separate (1.1) to the top, add the Constraint that ExecutionPolicy is an execution policy. (This indicates that the algorithm is a parallel algorithm.) (DONE)
[linalg.alg.blas3.rank2k]
Change linalg.alg to linalg.algs. (DONE)
Fix Effects generally. (DONE)
For next time
Next starting point is BLAS 3 TRSM.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the unchecked items in [mdspan.layout.policy.reqmts], the stride rank constraint, and the proposed layout_transpose rename. Compare the relevant layout wording and the LWG feedback in the issue, then resolve or document each remaining question; done means the outstanding checklist items are addressed and the review can resume at BLAS 3 TRSM.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- documentation
- Issue type
- Documentation
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100