#19045 closed enhancement (fixed)
better subs method for matrices
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.9 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Nathann Cohen, Thierry Monteil 
Report Upstream:  N/A  Work issues:  
Branch:  59990af (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
As mentioned on this sagedevel 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
Change History (15)
comment:1 Changed 4 years ago by
 Branch set to u/vdelecroix/19045
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit set to 07e07b83b6952d489ea719165ba1872fca444d40
comment:3 Changed 4 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
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
comment:4 Changed 4 years ago by
 Status changed from positive_review to needs_work
comment:5 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:6 Changed 4 years ago by
 Commit changed from 07e07b83b6952d489ea719165ba1872fca444d40 to fbcd9bc29fbcd1084168c8159449a8bf1450cdd0
Branch pushed to git repo; I updated commit sha1. New commits:
fbcd9bc  Trac #19045: a missing 's'

comment:7 Changed 4 years ago by
 Status changed from needs_review to positive_review
Thanks Nathann.
comment:8 Changed 4 years ago by
 Commit changed from fbcd9bc29fbcd1084168c8159449a8bf1450cdd0 to 59990af70ddf8f7de42d7c781b12248977eac664
 Status changed from positive_review to needs_review
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

comment:9 Changed 4 years ago by
 Reviewers changed from Nathann Cohen to Nathann Cohen, Thierry Monteil
 Status changed from needs_review to needs_work
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.
comment:10 Changed 4 years ago by
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
comment:11 Changed 4 years ago by
Replying to 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"?
comment:12 Changed 4 years ago by
 Status changed from needs_work to positive_review
I'm setting this back to positive_review
, as comment 9 is no reason to hold this branch.
Nathann
comment:13 Changed 4 years ago by
 Branch changed from u/vdelecroix/19045 to 59990af70ddf8f7de42d7c781b12248977eac664
 Resolution set to fixed
 Status changed from positive_review to closed
comment:14 followup: ↓ 15 Changed 4 years ago by
 Commit 59990af70ddf8f7de42d7c781b12248977eac664 deleted
 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.
comment:15 in reply to: ↑ 14 Changed 4 years ago by
Replying to 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 ;)
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #19045: better .subs() on matrices