[LWG 19] P1673R13 A free function linear algebra interface based on the BLAS #6704

jensmaurer · 2023-11-17T00:41:30Z

Fixes #6677
Fixes cplusplus/papers#557

Issues found and fixed in separate commits:

synopsis: missing closing } for namespace at end of synopsis
synopsis: T dot(...: "T" was never introduced; did you mean "Scalar"?
synopsis: template<class Scalar, class ExecutionPolicy, ...> void symmetric_matrix_rank_k_update: strange order of template parameters
There is lots of mentions of "BLAS", but no expansion of the acronym, and no bibliography entry for the BLAS specification.
[linalg.reqs.alg] p2.3 says "otherwise", but there is no preceding "if" that would match.
[linalg.layout.packed.obs] p3 (p13 in the paper https://wiki.edg.com/pub/Wg21kona2023/StrawPolls/P1673R13.html#wording ): p3 would benefit from having the "let" in p4 come earlier, so that it can be used.
The introductory notes for [linalg.helpers.mandates] vs. [linalg.helpers.precond] are different.
The specification for dotc seems overly complicated. In particular, the auto-returning functions construct a type "T",
which is a bit complicated. Instead, they could just say "return dot(conjugated(v1), v2)" and rely on the auto-returning "dot" function to do the right thing. NOT FIXED in this pull request
[linalg.algs.blas1.ssq] had a duplicate definition for struct sum_of_squares_result (also present in the synopsis).
[linalg.algs.blas1.ssq] talks about "LAPACK", but everywhere else, we talk about "BLAS". We need a bibliography entry for that.
Pervasive issue in the paper: It tries to convert "decltype(something)" to a type. You can't convert types to other types; you can only convert expressions to types. (Because the value category of the expression influences convertibility.) NOT FIXED in this pull request
The example scaled_matvec_2 in [linalg.algs.blas2.gemv] uses the "updating matrix-vector product" function that is defined later. I suggest to split the example to avoid the forward reference.
Some functions are available with an "alpha" scalar and without. Move functions with "alpha" first.
[linalg.algs.blas3.rankk] p7 (for hermitian_matrix_rank_k_update) does not show "alpha" in the formula of the effects. That feels like a bug.
[linalg.algs.blas3.rank2k] p6 (for hermitian_matrix_rank_2k_update) uses ^T, not ^H, in the formula. Is that correct?
There are a few cross-references to [linalg.algs], which doesn't exist. I've switched them to [linalg].

Dani-Hub · 2023-11-18T06:48:03Z

@jensmaurer The definition of imag-if-needed for sub-bullet (1.2) is incorrect, I assume that

otherwise, ((void)E, T).

should be either

otherwise, ((void)E, T{}).

as the paper says or

otherwise, ((void)E, T()).

jensmaurer · 2023-11-18T20:28:56Z

@Dani-Hub, thanks for the hint:

The definition of imag-if-needed for sub-bullet (1.2) is incorrect

Fixed; missing backslashes in LaTeX.

burblebee · 2023-11-18T21:55:03Z

@JohelEGP @jwakely (or any other volunteers) This one is huge - how about we split this review up between us? This is looks to be about fair:

28-28.9.2
28.9.3-28.9.13
28.9.14-end

I'll take the last one, 28.9.14-end.

burblebee

not done, but probably done for the night - here's what I have so far...

burblebee · 2023-11-18T22:41:01Z

source/numerics.tex

+constexpr size_t num_rows = 5;
+constexpr size_t num_cols = 6;
+
+// y = 3.0 * A * x


\tcode needed (same for C++ comments elsewhere)

Hm... operator* is not provided for the involved types, so formatting this as C++ code throughout isn't correct.
Formatting y, A, x as C++ code would work, but then this is almost indistinguishable from all-code.

@tkoeppe , what do you think?

We should probably write the code as code, but the whole expression as in maths. We can either use \times or \cdot or just juxtaposition for the various kinds of multiplication.

But we can clean that up later, too.

source/numerics.tex

burblebee · 2023-11-18T22:44:14Z

source/numerics.tex

+void scaled_matvec_1(mdspan<double, extents<size_t, num_rows, num_cols>> A,
+  mdspan<double, extents<size_t, num_cols>> x, mdspan<double, extents<size_t, num_rows>> y) {
+  matrix_vector_product(scaled(3.0, A), x, y);
+}


Format as it is in the paper - it's easier to see what's happening

I find all of this rather verbose. Maybe we should invent type aliases for these.
using Matrix = mdspan<double, extents<size_t, num_rows, num_cols>>;

@tkoeppe , what do you think?

Yes, that would be helpful. Let's do that as a follow-up.

burblebee · 2023-11-18T22:45:16Z

source/numerics.tex

+void scaled_matvec_2(mdspan<double, extents<size_t, num_rows, num_cols>> A,
+  mdspan<double, extents<size_t, num_cols>> x, mdspan<double, extents<size_t, num_rows>> y) {


Format as it is in the paper - it's easier to see what's happening

source/numerics.tex

JohelEGP · 2023-11-20T19:55:05Z

@JohelEGP @jwakely (or any other volunteers) This one is huge - how about we split this review up between us? This is looks to be about fair:
* 28-28.9.2

* 28.9.3-28.9.13

* 28.9.14-end
I'll take the last one, 28.9.14-end.

In that case, I'll take the first one.

burblebee

Thanks for the fixes so far!

Still have issues with:

\tcode vs. italics used to represent the same variables (in paper also),
spaces between variables in math expressions (looks like separate expressions, but without space as in paper, the string looks like a single variable), and
missing "is \tcode{true}" after some expressions (in paper also)

source/numerics.tex

burblebee · 2023-11-22T01:56:06Z

source/numerics.tex

+
+\pnum
+\effects
+Computes $z = y + A x$.


I disagree. At the very least, preface this section with what /A/, etc, are. But really, they are the variables in the prototypes, so why wouldn't we just use them directly? Also, having 2 variables right next to each other with only a space and no operator between them is meaningless in either math or C++ code. At least the paper didn't include a space, so could be read mathematically as a multiplication, but doing that gets confusing because the string of variables can look like a single variable

source/numerics.tex

jensmaurer · 2023-11-22T19:16:58Z

@jwakely , is there any specific text in the paper (that I'm currently overlooking) that would associate input math variables with same-named C++ function parameters? [linalg.general] p1.4 talks about output variables only.

burblebee

Just 1 last required change

burblebee · 2023-11-22T21:03:18Z

source/numerics.tex

+Computes a vector $x'$ such that $b = A x'$,
+and assigns each element of $x'$ to the corresponding element of $x$.
+If no such $x'$ exists,
+then the elements of \tcode{x} are valid but unspecified.


This x should be in math font (not \tcode) for consistency

jwakely · 2023-12-18T00:20:44Z

@jwakely , is there any specific text in the paper (that I'm currently overlooking) that would associate input math variables with same-named C++ function parameters? [linalg.general] p1.4 talks about output variables only.

Erm, possibly not. We have similar problems in [rand], see #4295 for an old attempt to fix it there.

tkoeppe · 2023-12-18T02:37:19Z

Thank you all very much for the reviews, and special thanks to Jens for the drafting! I'll merge this now, and we can perform follow-up cleanup afterwards. Thanks again!

- Add closing brace in header synopsis at the end of the namespace. - Introduce abbreviations for repeated lengthy expressions. - Remove duplicate definition for struct sum_of_squares_result from [linalg.algs.blas1.ssq]. - Stale cross-references to [linalg.algs] re-pointed to [linalg].

…_update

…xpects

… with alpha first

jensmaurer added this to the post-2023-11 milestone Nov 17, 2023

jensmaurer force-pushed the motions-2023-11-lwg-19 branch from ae27623 to c8710ca Compare November 17, 2023 10:21

jensmaurer changed the title ~~P1673R13 A free function linear algebra interface based on the BLAS~~ [LWG 19] P1673R13 A free function linear algebra interface based on the BLAS Nov 17, 2023

jensmaurer force-pushed the motions-2023-11-lwg-19 branch 2 times, most recently from 3a8aa93 to aa6d7e9 Compare November 17, 2023 21:37

jensmaurer marked this pull request as ready for review November 17, 2023 22:09

jensmaurer force-pushed the motions-2023-11-lwg-19 branch from aa6d7e9 to 47b2f70 Compare November 17, 2023 22:11

jensmaurer force-pushed the motions-2023-11-lwg-19 branch 2 times, most recently from 1e16e8c to 51afb68 Compare November 18, 2023 20:02

burblebee requested changes Nov 19, 2023

View reviewed changes

burblebee requested changes Nov 22, 2023

View reviewed changes

jensmaurer force-pushed the motions-2023-11-lwg-19 branch from f3d4d06 to d695241 Compare November 22, 2023 19:15

burblebee requested changes Nov 22, 2023

View reviewed changes

tkoeppe force-pushed the motions-2023-11-lwg-19 branch from d695241 to e7b98fd Compare December 17, 2023 23:09

tkoeppe requested a review from burblebee December 17, 2023 23:22

tkoeppe force-pushed the motions-2023-11-lwg-19 branch from e7b98fd to 731356d Compare December 18, 2023 02:11

jensmaurer added 7 commits December 18, 2023 02:39

[linalg.syn] Fix return type of the dot function

dac6839

[linalg] Fix order of template parameters for symmetric_matrix_rank_k…

7f248b8

…_update

[linalg.reqs.alg] Clarify requirements for other than solve algorithms

764f4fd

[linalg.layout.packed.obs] Introduce i and j earlier and use it in \e…

fb16d59

…xpects

[linalg.helpers.precond] Align note with [linalg.helpers.mandates]

3130bf4

[linalg.algs.blas2.gemv] Move example to avoid forward reference

f7c04aa

jensmaurer added 8 commits December 18, 2023 02:39

[linalg.helpers] Remove redundancies from subordinate headings

c9d19d4

[linalg.algs.blas3.rankk] Fix missing alpha in formula

3aa2e6c

[linalg.algs.blas3.rank2k] Fix missing conjugate in formula

e337d13

[linalg.general] Use E as a placeholder for an expression

9718fea

[linalg.syn,linalg.algs.blas2] Reorder presentation to show overloads…

9751cf1

… with alpha first

[bib] Add bibliography items for BLAS and LAPACK

769f82e

[std] Use numbered bibliography references throughout the standard

aa595cb

[linalg.algs.blas1] Add references to the 'BLAS standard'

1945031

tkoeppe force-pushed the motions-2023-11-lwg-19 branch from 731356d to 1945031 Compare December 18, 2023 02:39

tkoeppe merged commit 7f97465 into main Dec 18, 2023
4 checks passed

jensmaurer deleted the motions-2023-11-lwg-19 branch March 23, 2024 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LWG 19] P1673R13 A free function linear algebra interface based on the BLAS #6704

[LWG 19] P1673R13 A free function linear algebra interface based on the BLAS #6704

jensmaurer commented Nov 17, 2023 •

edited

Loading

Dani-Hub commented Nov 18, 2023

jensmaurer commented Nov 18, 2023

burblebee commented Nov 18, 2023

burblebee left a comment

burblebee Nov 18, 2023 •

edited

Loading

jensmaurer Nov 19, 2023

tkoeppe Dec 18, 2023

burblebee Nov 18, 2023

jensmaurer Nov 19, 2023

tkoeppe Dec 18, 2023

burblebee Nov 18, 2023

JohelEGP commented Nov 20, 2023

burblebee left a comment •

edited

Loading

burblebee Nov 22, 2023 •

edited

Loading

jensmaurer commented Nov 22, 2023

burblebee left a comment

burblebee Nov 22, 2023 •

edited

Loading

jwakely commented Dec 18, 2023

tkoeppe commented Dec 18, 2023

		void scaled_matvec_2(mdspan<double, extents<size_t, num_rows, num_cols>> A,
		mdspan<double, extents<size_t, num_cols>> x, mdspan<double, extents<size_t, num_rows>> y) {

[LWG 19] P1673R13 A free function linear algebra interface based on the BLAS #6704

[LWG 19] P1673R13 A free function linear algebra interface based on the BLAS #6704

Conversation

jensmaurer commented Nov 17, 2023 • edited Loading

Dani-Hub commented Nov 18, 2023

jensmaurer commented Nov 18, 2023

burblebee commented Nov 18, 2023

burblebee left a comment

Choose a reason for hiding this comment

burblebee Nov 18, 2023 • edited Loading

Choose a reason for hiding this comment

jensmaurer Nov 19, 2023

Choose a reason for hiding this comment

tkoeppe Dec 18, 2023

Choose a reason for hiding this comment

burblebee Nov 18, 2023

Choose a reason for hiding this comment

jensmaurer Nov 19, 2023

Choose a reason for hiding this comment

tkoeppe Dec 18, 2023

Choose a reason for hiding this comment

burblebee Nov 18, 2023

Choose a reason for hiding this comment

JohelEGP commented Nov 20, 2023

burblebee left a comment • edited Loading

Choose a reason for hiding this comment

burblebee Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

jensmaurer commented Nov 22, 2023

burblebee left a comment

Choose a reason for hiding this comment

burblebee Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

jwakely commented Dec 18, 2023

tkoeppe commented Dec 18, 2023

jensmaurer commented Nov 17, 2023 •

edited

Loading

burblebee Nov 18, 2023 •

edited

Loading

burblebee left a comment •

edited

Loading

burblebee Nov 22, 2023 •

edited

Loading

burblebee Nov 22, 2023 •

edited

Loading