Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#19045 closed enhancement (fixed)

better subs method for matrices

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.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 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

Change History (15)

comment:1 Changed 4 years ago by vdelecroix

  • Branch set to u/vdelecroix/19045
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to 07e07b83b6952d489ea719165ba1872fca444d40

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

07e07b8Trac #19045: better .subs() on matrices

comment:3 Changed 4 years ago by ncohen

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

  • Status changed from positive_review to needs_work

comment:5 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:6 Changed 4 years ago by git

  • Commit changed from 07e07b83b6952d489ea719165ba1872fca444d40 to fbcd9bc29fbcd1084168c8159449a8bf1450cdd0

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

fbcd9bcTrac #19045: a missing 's'

comment:7 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Thanks Nathann.

comment:8 Changed 4 years ago by git

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

59990afTrac #19045: keep sparsity + more doc

comment:9 Changed 4 years ago by tmonteil

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

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 vdelecroix

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 ncohen

  • 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

Last edited 4 years ago by ncohen (previous) (diff)

comment:13 Changed 4 years ago by vbraun

  • Branch changed from u/vdelecroix/19045 to 59990af70ddf8f7de42d7c781b12248977eac664
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 follow-up: Changed 4 years ago by tmonteil

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

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 ;-)

Note: See TracTickets for help on using tickets.