Documentation preview for this PR (built with commit 0b29b37; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

I would say we should make the usual identification of vectors as $1 \times k$ matrices for this construction. So the implementation just needs a bit more care (or just modify your loop to convert them to matrices for a quick fix).

I would say we should make the usual identification of vectors as 1 × k matrices for this construction. So the implementation just needs a bit more care (or just modify your loop to convert them to matrices for a quick fix).

I think vectors are more commonly considered $k × 1$ matrices. On the other hand, calling .row() or .column() on a vector is short enough.

I would say we should make the usual identification of vectors as 1 × k matrices for this construction. So the implementation just needs a bit more care (or just modify your loop to convert them to matrices for a quick fix).

I think vectors are more commonly considered k × 1 matrices.

Yes, I completely agree. That was a typo on my part.

On the other hand, calling .row() or .column() on a vector is short enough.

True, but I would say a similar sort of thing is done for the scalars. Although I guess from the matrices, one could deduce if it was meant as a row or column vector. Well, I guess erroring out is okay… But perhaps we should instead check only for scalars and matrices (rather than only rule out vectors)?

Yes, I completely agree. That was a typo on my part.

Maybe we can get by with considering them as column vector after all.

But perhaps we should instead check only for scalars and matrices (rather than only rule out vectors)?

Reasonable. But since the base ring may not be a priori known, we could check that they are ring elements instead.

(also, matrices are ring elements.)

I thought isinstance(M, RingElement) would check if it's a scalar, but there are Python types too. I think we can overlook that because of duck typing.

Or is it better to use type_to_parent? The advantage is clearer error message for users, the disadvantage is more complicated code. I'm thinking of something like

diff --git a/src/sage/matrix/special.py b/src/sage/matrix/special.py
index 375cb0c3234..894cd26d457 100644
--- a/src/sage/matrix/special.py
+++ b/src/sage/matrix/special.py
@@ -2096,7 +2096,12 @@ def block_matrix(*args, **kwds):
             for M in row:
                 R = M.base_ring() if isinstance(M, Matrix) else parent(M)
                 if R is not ZZ:
-                    ring = sage.categories.pushout.pushout(ring, R)
+                    try:
+                        ring = sage.categories.pushout.pushout(ring, R)
+                    except:
+                        if not isinstance(M, Matrix) and type_to_parent(parent(M)) not in Rings():
+                            raise ValueError(f"{M} must be a scalar, vector or matrix")
+                        raise
 
     if sparse is None:
         sparse = True

(on an unrelated note, it's probably better to use common_parent instead of this)

I thought isinstance(M, RingElement) would check if it's a scalar, but there are Python types too. I think we can overlook that because of duck typing.

Indeed, that is not a good test as many (commutative) ring elements do not inherit from this class (in fact, it would make things a bit easier for us to get rid of that class and just use Element IMO).

Or is it better to use type_to_parent? The advantage is clearer error message for users, the disadvantage is more complicated code.

My initial reaction was to keep the code simple. Yet, this is such a fundamental function (in the sense of the mathematics where 1st year students could use it), so my current belief is we should have the more clear error message. Do you want to make that change on this PR as well?

(on an unrelated note, it's probably better to use common_parent instead of this)

Yea, probably common_parent is better.

so something like this? If we want to go the whitelist (instead of blacklist) route

# assume is neither matrix or vector
R = parent(M)
if isinstance(R, type): R = type_to_parent(R)
if R in Rings(): ...

there might not be that large of an improvement though, since other than vectors what other invalid things could people reasonably pass in? (str? list? tuple?)

It shouldn't know how to process those things currently, right? So it is at least implicitly a whitelist as implemented; at least that is my interpretation from briefly looking at the code. I might be mistaken though.