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: sage-6.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:

Status badges

Description (last modified by jdemeyer)

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

Change History (20)

comment:1 Changed 7 years ago by chapoton

  • Branch set to u/chapoton/16399
  • Commit set to 098d574aa98441e4067178a4b8913c88ebc0f6b8

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:

7b18078trac #16399 first naive try
098d574trac #16399 now for sparse matrices

comment:2 Changed 7 years ago by 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

comment:3 Changed 7 years ago by git

  • Commit changed from 098d574aa98441e4067178a4b8913c88ebc0f6b8 to bee1968a7bfc534ac1ce216562b8f66df770483e

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

bee1968trac #16399 trying to use coercion model

comment:4 Changed 7 years ago by git

  • Commit changed from bee1968a7bfc534ac1ce216562b8f66df770483e to 9986c476c78510e1b98d46890a5b9506f3b62613

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

9986c47trac #16399 fixing some obvious errors.

comment:5 follow-up: Changed 7 years ago by 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.

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:7 in reply to: ↑ 5 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by 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.

comment:9 Changed 6 years ago by jdemeyer

  • 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 git

  • Commit changed from 9986c476c78510e1b98d46890a5b9506f3b62613 to 0f1cbae5d63bc2005412f4b2870f7769185252e7

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

0f1cbaeMerge tag '6.5.beta5' into ticket/16399

comment:11 Changed 6 years ago by git

  • Commit changed from 0f1cbae5d63bc2005412f4b2870f7769185252e7 to bf6abd1d1f6203dad05323a4bf0192544861d72b

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

bf6abd1Refactor stacking, use coercion

comment:12 Changed 6 years ago by git

  • Commit changed from bf6abd1d1f6203dad05323a4bf0192544861d72b to e3d7bbd479838b2cbc8433509034da0ab864d456

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

e3d7bbdRefactor stacking, use coercion

comment:13 Changed 6 years ago by jdemeyer

  • Authors set to Frédéric Chapoton, Jeroen Demeyer
  • Status changed from new to needs_review

comment:14 follow-up: Changed 6 years ago by 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

    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): 
    423423
    424424            # Stack it on top of the basis for W'.
    425425            Wp = CD.V().coordinate_module(CD.W()).basis_matrix()
     426            Wp = Wp.change_ring(A.base_ring())
    426427            B = A.stack(Wp)
    427428
    428429            # 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 ; follow-up: Changed 6 years ago by jdemeyer

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): 
    423423
    424424            # Stack it on top of the basis for W'.
    425425            Wp = CD.V().coordinate_module(CD.W()).basis_matrix()
     426            Wp = Wp.change_ring(A.base_ring())
    426427            B = A.stack(Wp)
    427428
    428429            # 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 git

  • Commit changed from e3d7bbd479838b2cbc8433509034da0ab864d456 to 810a889f723ae78ee0331cf366e22e2308ebf74e

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

810a889Rephrase documentation

comment:17 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-6.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 jdemeyer

  • Component changed from coercion to linear algebra
  • Description modified (diff)

comment:19 in reply to: ↑ 15 Changed 6 years ago by tscrim

  • 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 vbraun

  • Branch changed from u/jdemeyer/ticket/16399 to 810a889f723ae78ee0331cf366e22e2308ebf74e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.