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: sage-6.4
Component: linear algebra Keywords: simplify_full matrix
Cc: kcrisman Merged in:
Authors: Shashank Shalgar, Michael Orlitzky Reviewers: Karl-Dieter Crisman, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 4040117 (Commits, GitHub, GitLab) Commit: 4040117e8d920d235e6b02ca4c38bfd1f5da9880
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

A minor change to implement simplify_full for matrix.

Attachments (1)

trac_12162_simplify_full_matrix.patch (1015 bytes) - added by Shashank 9 years ago.

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by Shashank

comment:1 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:2 Changed 8 years ago by knsam

  • Status changed from new to needs_review

I am putting this to needs_review as there is an attachment which is more or less complete.

comment:3 Changed 8 years ago by jdemeyer

Please add your real name as Author.

comment:4 Changed 8 years ago by kcrisman

  • Authors set to Shashank Shalgar

I think it's this user at ask.sagemath. The name in the patch confirms it.

Last edited 8 years ago by kcrisman (previous) (diff)

comment:5 Changed 8 years ago by kcrisman

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 kcrisman

  • Status changed from needs_review to needs_info

comment:7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:11 Changed 7 years ago by mjo

  • Authors changed from Shashank Shalgar to Shashank Shalgar, Michael Orlitzky
  • 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 two-line implementation with the one-line implementation based on that ticket.

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

comment:13 in reply to: ↑ 12 Changed 6 years ago by vdelecroix

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

  • Commit changed from caaf80ea42bf4f38878cfe07079ba9df84b855db to 4040117e8d920d235e6b02ca4c38bfd1f5da9880

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

4040117Trac #12162: Add a simplify_full() method to the Matrix_symbolic_dense class.

comment:15 follow-up: Changed 6 years ago by mjo

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

  • Description modified (diff)
  • Reviewers set to Karl-Dieter Crisman, Vincent Delecroix
  • Status changed from needs_review to positive_review

Good!

comment:17 in reply to: ↑ 15 Changed 6 years ago by vdelecroix

Replying to mjo:

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

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 vbraun

  • Branch changed from u/mjo/ticket/12162 to 4040117e8d920d235e6b02ca4c38bfd1f5da9880
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.