Opened 9 years ago
Closed 6 years ago
#12162 closed enhancement (fixed)
simplify_full for matrix
Reported by:  Shashank  Owned by:  jason, was 

Priority:  trivial  Milestone:  sage6.4 
Component:  linear algebra  Keywords:  simplify_full matrix 
Cc:  kcrisman  Merged in:  
Authors:  Shashank Shalgar, Michael Orlitzky  Reviewers:  KarlDieter Crisman, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  4040117 (Commits, GitHub, GitLab)  Commit:  4040117e8d920d235e6b02ca4c38bfd1f5da9880 
Dependencies:  Stopgaps: 
Description (last modified by )
A minor change to implement simplify_full
for matrix.
Attachments (1)
Change History (19)
Changed 9 years ago by
comment:1 Changed 8 years ago by
 Cc kcrisman added
comment:2 Changed 8 years ago by
 Status changed from new to needs_review
comment:3 Changed 8 years ago by
Please add your real name as Author.
comment:4 Changed 8 years ago by
I think it's this user at ask.sagemath. The name in the patch confirms it.
comment:5 Changed 8 years ago by
Also, it's not clear whether we should perhaps focus attention on #10552 instead. I feel like the approach there makes more sense, using apply_map
.
comment:6 Changed 8 years ago by
 Status changed from needs_review to needs_info
comment:7 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:8 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:9 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:10 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:11 Changed 7 years ago by
 Branch set to u/mjo/ticket/12162
 Commit set to caaf80ea42bf4f38878cfe07079ba9df84b855db
 Status changed from needs_info to needs_review
We've been waiting for a perfect solution for ~3 years, maybe it's time to merge the obvious implementation? If anyone does the work on #10552, all we'd have to do is replace this twoline implementation with the oneline implementation based on that ticket.
comment:12 followup: ↓ 13 Changed 6 years ago by
You're right, of course. Some of the other methods do
return self.parent()([x.simplify() for x in self.list()])
for what it's worth.
comment:13 in reply to: ↑ 12 Changed 6 years ago by
 Status changed from needs_review to needs_work
Replying to kcrisman:
You're right, of course. Some of the other methods do
return self.parent()([x.simplify() for x in self.list()])for what it's worth.
I do not understand why the method .matrix_space()
even exists! It is infinitely slower and should in all cases return the same thing as .parent()
. It might be useful if a class inherit from MatrixSpace
but I do not think it is the case. It would better be changed.
comment:14 Changed 6 years ago by
 Commit changed from caaf80ea42bf4f38878cfe07079ba9df84b855db to 4040117e8d920d235e6b02ca4c38bfd1f5da9880
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4040117  Trac #12162: Add a simplify_full() method to the Matrix_symbolic_dense class.

comment:15 followup: ↓ 17 Changed 6 years ago by
 Status changed from needs_work to needs_review
I went with matrix_space()
because I've never really known what parent()
is supposed to do. But I guess it's safe since everywhere else in the class, "parent" means "immediate parent matrix space." I also changed the implementation to use a list comprehension (no reason, my personal taste has changed).
comment:16 Changed 6 years ago by
 Description modified (diff)
 Reviewers set to KarlDieter Crisman, Vincent Delecroix
 Status changed from needs_review to positive_review
Good!
comment:17 in reply to: ↑ 15 Changed 6 years ago by
Replying to mjo:
I went with
matrix_space()
because I've never really known whatparent()
is supposed to do. But I guess it's safe since everywhere else in the class, "parent" means "immediate parent matrix space." I also changed the implementation to use a list comprehension (no reason, my personal taste has changed).
Most objects have a parent. That is the set in which they belong:
sage: 1.parent() Integer Ring sage: Permutation([3,2,1]).parent() Standard permutations sage: (1.3).parent() Real Field with 53 bits of precision
This is coded at very low level (namely sage.structure.element
), and hence very fast. From the parent, you should be able to build new element of the same set.
comment:18 Changed 6 years ago by
 Branch changed from u/mjo/ticket/12162 to 4040117e8d920d235e6b02ca4c38bfd1f5da9880
 Resolution set to fixed
 Status changed from positive_review to closed
I am putting this to needs_review as there is an attachment which is more or less complete.