Matrix stack() should coerce to a common parent

[tscrim]

I feel like we shouldn't have to do an explicit ring change to do this. Plus we get (somewhat) different failures for dense versus sparse matrices:

sage: m = matrix([[1,2]])
sage: m2 = matrix(QQ, [[1/2,2]])
sage: m.stack(m2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: matrix has denominators so can't change to ZZ.

sage: m = matrix([[1,2]], sparse=True)
sage: m.stack(m2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: no conversion of this rational to integer

Follow-up: #17595

[chapoton]

I have tried to look at the code. Apparently, one only tries to change the base ring of the bottom matrix. But of course, the coercion could be the other way, or there could be some complicated way to find a common coercion.

I have made a preliminary attempt, very incomplete.

---

New commits:

​7b18078	trac #16399 first naive try
​098d574	trac #16399 now for sparse matrices

[tscrim]

Here's how to get a common parent:

sage: from sage.structure.element import get_coercion_model
sage: CM = get_coercion_model()
sage: CM.common_parent(QQ, ZZ['x'])
Univariate Polynomial Ring in x over Rational Field

and since we're modifying cython files, we could probably use the cdef global coercion model from element.pyx:

cdef CoercionModel coercion_model

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​bee1968

trac #16399 trying to use coercion model

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​9986c47

trac #16399 fixing some obvious errors.

[nbruin]

Given the syntax, m.stack(...) it makes a lot of sense to try and let the result depend as much as possible on m and not on "...". The error you get now is quick and it is clear how to avoid it. With coercion, you could get an unexpected base ring change of the resulting matrix, which might give erroneous results much later.

The example below gives questionable results, but the change you propose here would break the behaviour we have already:

sage: M=matrix(ZZ,[1]).stack(matrix(GF(3),[1])).stack(matrix(GF(5),[1]))
sage: M.base_ring()
Integer Ring

Clearly, the current semantics are *conversion* into the base ring of the first matrix. Changing that into *coercion* into a common parent would be a real change, and it's not clear to me the resulting semantics are entirely desirable.

I'm not particularly defending the current semantics either. I'm just pointing out you're proposing an incompatible change and for that to be justified we'd need fairly wide concensus that the change leads to significantly more desirable behaviour.

[jdemeyer]

Replying to nbruin:

Given the syntax, m.stack(...) it makes a lot of sense to try and let the result depend as much as possible on m and not on "...".

I disagree with this. That's just a pure syntactical thing, mathematically "stacking" could be seen as a binary operator.

I am +1 to coercion, but there could indeed be unexpected consequences.

[jdemeyer]

Note: I hit the issue on this ticket by working on #17585. Apparently, the basis_matrix() method of modules with basis always returns a matrix over the fraction field. Changing this gives errors because of the issue here.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​0f1cbae

Merge tag '6.5.beta5' into ticket/16399

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​bf6abd1

Refactor stacking, use coercion

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​e3d7bbd

Refactor stacking, use coercion

[tscrim]

Looks good overall, but there are two things.

The first is could you keep the "returns a new matrix" part of the documentation? Since matrices are mutable, your proposed change makes the result slightly ambiguous to me as we could mutate in place and return self.

Second is about this change:

src/sage/modules/fg_pid/fgp_morphism.py

@@ -423 +423 @@
 
             # Stack it on top of the basis for W'.
             Wp = CD.V().coordinate_module(CD.W()).basis_matrix()
+            Wp = Wp.change_ring(A.base_ring())
             B = A.stack(Wp)
 
             # Compute Hermite form of C with transformation

Is this necessary for this ticket or is it suppose to be a part of #17585?

[jdemeyer]

Replying to tscrim:

src/sage/modules/fg_pid/fgp_morphism.py

@@ -423 +423 @@
 
             # Stack it on top of the basis for W'.
             Wp = CD.V().coordinate_module(CD.W()).basis_matrix()
+            Wp = Wp.change_ring(A.base_ring())
             B = A.stack(Wp)
 
             # Compute Hermite form of C with transformation

Is this necessary for this ticket or is it suppose to be a part of #17585?

Yes, it's necessary for this ticket but it's not needed with #16399 and #17585 together. So it could be added here and removed again in #17585.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​810a889

Rephrase documentation

[tscrim]

Replying to jdemeyer:

Is this necessary for this ticket or is it suppose to be a part of #17585?

Yes, it's necessary for this ticket but it's not needed with #16399 and #17585 together. So it could be added here and removed again in #17585.

Okay, then positive review. Thanks for making that change to the doc.