better subs method for matrices

[videlec]

As mentioned on this sage-devel thread the .subs() method of matrices behaves badly with polynomials.

sage: x = polygen(ZZ)
sage: matrix([[x]]).subs(x=1)
Traceback (most recent call last):
...
ValueError: must not specify both a keyword and positional argument
sage: x.subs(1).parent()
Integer Ring
sage: matrix([[x]]).subs(1).parent()
Full MatrixSpace of 1 by 1 dense matrices over Univariate
Polynomial Ring in x over Integer Ring

Component: linear algebra

Author: Vincent Delecroix

Branch: 59990af

Reviewer: Nathann Cohen, Thierry Monteil

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

[videlec]

Branch: u/vdelecroix/19045

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

07e07b8

Trac #19045: better .subs() on matrices

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Commit: 07e07b8

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

Reviewer: Nathann Cohen

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

comment:3

Looks good. What about 'does on coefficient': isn't a 's' missing there?

Otherwise it's good. Regardless of what you choose to do with this, you can set the ticket to positive_review on my behalf.

Nathann

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 07e07b8 to fbcd9bc

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

fbcd9bc

Trac #19045: a missing 's'

[videlec]

comment:7

Thanks Nathann.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

59990af

Trac #19045: keep sparsity + more doc

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from fbcd9bc to 59990af

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:9

The current situation is not homogeneous:

sage: R.<x> = ZZ[]
sage: M = matrix([[x]])
sage: M.subs({x:1}).parent()
Full MatrixSpace of 1 by 1 dense matrices over Integer Ring

sage: R.<x,y,z> = ZZ[]
sage: M = matrix([[x]])
sage: M.subs({x:1}).parent()
Full MatrixSpace of 1 by 1 dense matrices over Multivariate Polynomial Ring in x, y, z over Integer Ring

This is probably due to a problem in the substitution at the polynomial level.

Also, the parent of the result should be determined by the parent of the matrix and the parent of the substitued values, not only the entries of the result, or it will be unpredictable.

[edd8e884-f507-429a-b577-5d554626c0fe]

Changed reviewer from Nathann Cohen to Nathann Cohen, Thierry Monteil

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:10

Even worse:

sage: R.<x,y,z> = ZZ[]
sage: M = matrix([[x]])
sage: M.subs({x:1}).parent()
Full MatrixSpace of 1 by 1 dense matrices over Multivariate Polynomial Ring in x, y, z over Integer Ring
sage: M.subs({x:RDF(1)}).parent()
Full MatrixSpace of 1 by 1 dense matrices over Real Double Field

[videlec]

comment:11

Replying to @sagetrac-tmonteil:

This is probably due to a problem in the substitution at the polynomial level.

It is. And then indepent of this ticket.

sage: R.<x,y,z> = ZZ[]
sage: x.subs(x=1).parent()
Multivariate Polynomial Ring in x, y, z over Integer Ring
sage: x.subs(x=RDF(1)).parent()
Real Double Field

The above subs is clearly broken and you are welcome to fix it.

The method matrix.subs() is just a shortcut to apply subs to all coefficients. It is needed because otherwise there is the one that is inherited from Element.

Also, the parent of the result should be determined by the parent of the matrix and the parent of the substitued values, not only the entries of the result, or it will be unpredictable.

I do not see what I can do better for the sake of this ticket. This is not clear enough to me what I should do. Perhaps opening a ticket "give specifications for subs and include it in the coercion model"?

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

comment:12

I'm setting this back to positive_review, as comment [comment:9] is no reason to hold this branch.

Nathann

[vbraun]

Changed branch from u/vdelecroix/19045 to 59990af

[edd8e884-f507-429a-b577-5d554626c0fe]

Changed commit from 59990af to none

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:14

• the problem this ticket is supposed to address is about the .subs() method changing the parent (see the linked thread). Is it fixed ?

• the problem actually comes from an inconsistent subs for polynomials. This is usually called a dependency, and there is even a field for that on trac, so that we ensure the actual problem get solved.

• having ticket merged that fast is a good way not to fix the real problem.

[videlec]

comment:15

Replying to @sagetrac-tmonteil:

• the problem this ticket is supposed to address is about the .subs() method changing the parent (see the linked thread). Is it fixed ?

Yes

sage: R.<x> = PolynomialRing(ZZ) 
sage: m = matrix(R, [[x]]) 
sage: m.subs(3).parent()
Full MatrixSpace of 1 by 1 dense matrices over Integer Ring

• the problem actually comes from an inconsistent subs for polynomials. This is usually called a dependency, and there is even a field for that on trac, so that we ensure the actual problem get solved.

This is not a dependency. But I opened #19130.

• having ticket merged that fast is a good way not to fix the real problem.

not merging ticket is also a good way to not fix it ;-)