Opened 7 years ago
Closed 6 years ago
#16399 closed defect (fixed)
Matrix stack() should coerce to a common parent
Reported by:  tscrim  Owned by:  tscrim 

Priority:  major  Milestone:  sage6.5 
Component:  linear algebra  Keywords:  matrix stack coercion 
Cc:  Merged in:  
Authors:  Frédéric Chapoton, Jeroen Demeyer  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  810a889 (Commits, GitHub, GitLab)  Commit:  810a889f723ae78ee0331cf366e22e2308ebf74e 
Dependencies:  Stopgaps: 
Description (last modified by )
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
Followup: #17595
Change History (20)
comment:1 Changed 7 years ago by
 Branch set to u/chapoton/16399
 Commit set to 098d574aa98441e4067178a4b8913c88ebc0f6b8
comment:2 Changed 7 years ago by
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
comment:3 Changed 7 years ago by
 Commit changed from 098d574aa98441e4067178a4b8913c88ebc0f6b8 to bee1968a7bfc534ac1ce216562b8f66df770483e
Branch pushed to git repo; I updated commit sha1. New commits:
bee1968  trac #16399 trying to use coercion model

comment:4 Changed 7 years ago by
 Commit changed from bee1968a7bfc534ac1ce216562b8f66df770483e to 9986c476c78510e1b98d46890a5b9506f3b62613
Branch pushed to git repo; I updated commit sha1. New commits:
9986c47  trac #16399 fixing some obvious errors.

comment:5 followup: ↓ 7 Changed 7 years ago by
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.
comment:6 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:7 in reply to: ↑ 5 Changed 6 years ago by
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.
comment:8 Changed 6 years ago by
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.
comment:9 Changed 6 years ago by
 Branch changed from u/chapoton/16399 to u/jdemeyer/ticket/16399
 Created changed from 05/26/14 03:05:59 to 05/26/14 03:05:59
 Modified changed from 01/06/15 11:39:54 to 01/06/15 11:39:54
comment:10 Changed 6 years ago by
 Commit changed from 9986c476c78510e1b98d46890a5b9506f3b62613 to 0f1cbae5d63bc2005412f4b2870f7769185252e7
Branch pushed to git repo; I updated commit sha1. New commits:
0f1cbae  Merge tag '6.5.beta5' into ticket/16399

comment:11 Changed 6 years ago by
 Commit changed from 0f1cbae5d63bc2005412f4b2870f7769185252e7 to bf6abd1d1f6203dad05323a4bf0192544861d72b
Branch pushed to git repo; I updated commit sha1. New commits:
bf6abd1  Refactor stacking, use coercion

comment:12 Changed 6 years ago by
 Commit changed from bf6abd1d1f6203dad05323a4bf0192544861d72b to e3d7bbd479838b2cbc8433509034da0ab864d456
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e3d7bbd  Refactor stacking, use coercion

comment:13 Changed 6 years ago by
 Status changed from new to needs_review
comment:14 followup: ↓ 15 Changed 6 years ago by
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
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 b class FGP_Morphism(Morphism): 423 423 424 424 # Stack it on top of the basis for W'. 425 425 Wp = CD.V().coordinate_module(CD.W()).basis_matrix() 426 Wp = Wp.change_ring(A.base_ring()) 426 427 B = A.stack(Wp) 427 428 428 429 # Compute Hermite form of C with transformation
Is this necessary for this ticket or is it suppose to be a part of #17585?
comment:15 in reply to: ↑ 14 ; followup: ↓ 19 Changed 6 years ago by
Replying to tscrim:
src/sage/modules/fg_pid/fgp_morphism.py
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 b class FGP_Morphism(Morphism): 423 423 424 424 # Stack it on top of the basis for W'. 425 425 Wp = CD.V().coordinate_module(CD.W()).basis_matrix() 426 Wp = Wp.change_ring(A.base_ring()) 426 427 B = A.stack(Wp) 427 428 428 429 # 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.
comment:16 Changed 6 years ago by
 Commit changed from e3d7bbd479838b2cbc8433509034da0ab864d456 to 810a889f723ae78ee0331cf366e22e2308ebf74e
Branch pushed to git repo; I updated commit sha1. New commits:
810a889  Rephrase documentation

comment:17 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.5
 Summary changed from Matrix stack doesn't coerce to a common parent to Matrix stack() should coerce to a common parent
comment:18 Changed 6 years ago by
 Component changed from coercion to linear algebra
 Description modified (diff)
comment:19 in reply to: ↑ 15 Changed 6 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
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.
comment:20 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/16399 to 810a889f723ae78ee0331cf366e22e2308ebf74e
 Resolution set to fixed
 Status changed from positive_review to closed
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:
trac #16399 first naive try
trac #16399 now for sparse matrices