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

Component: linear algebra

Keywords: matrix stack coercion

Author: Frédéric Chapoton, Jeroen Demeyer

Branch/Commit: 810a889

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/16399

[fchapoton]

Commit: 098d574

[fchapoton]

Branch: u/chapoton/16399

[fchapoton]

comment:1

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]

comment:2

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

[sagetrac-git]

Changed commit from 098d574 to bee1968

[sagetrac-git]

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

bee1968

trac #16399 trying to use coercion model

[sagetrac-git]

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

9986c47

trac #16399 fixing some obvious errors.

[sagetrac-git]

Changed commit from bee1968 to 9986c47

[nbruin]

comment:5

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]

comment:7

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]

comment:8

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.

[jdemeyer]

Changed branch from u/chapoton/16399 to u/jdemeyer/ticket/16399

[sagetrac-git]

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

0f1cbae

Merge tag '6.5.beta5' into ticket/16399

[sagetrac-git]

Changed commit from 9986c47 to 0f1cbae

[sagetrac-git]

Changed commit from 0f1cbae to bf6abd1

[sagetrac-git]

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

bf6abd1

Refactor stacking, use coercion

[sagetrac-git]

Changed commit from bf6abd1 to e3d7bbd

[sagetrac-git]

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

e3d7bbd

Refactor stacking, use coercion

[jdemeyer]

Author: Frédéric Chapoton, Jeroen Demeyer

[tscrim]

comment:14

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:

diff --git a/src/sage/modules/fg_pid/fgp_morphism.py b/src/sage/modules/fg_pid/fgp_morphism.py
index 80fe02b..86bbe37 100644
--- a/src/sage/modules/fg_pid/fgp_morphism.py
+++ b/src/sage/modules/fg_pid/fgp_morphism.py
@@ -423,6 +423,7 @@ class FGP_Morphism(Morphism):
 
             # 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]

comment:15

Replying to @tscrim:

diff --git a/src/sage/modules/fg_pid/fgp_morphism.py b/src/sage/modules/fg_pid/fgp_morphism.py
index 80fe02b..86bbe37 100644
--- a/src/sage/modules/fg_pid/fgp_morphism.py
+++ b/src/sage/modules/fg_pid/fgp_morphism.py
@@ -423,6 +423,7 @@ class FGP_Morphism(Morphism):
 
             # 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.

[sagetrac-git]

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

810a889

Rephrase documentation

[sagetrac-git]

Changed commit from e3d7bbd to 810a889

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:19

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.

[vbraun]

Changed branch from u/jdemeyer/ticket/16399 to 810a889