Opened 8 months ago

Closed 8 months ago

#31254 closed enhancement (fixed)

Do not perform unnecessary subdivisions in matrices

Reported by: tscrim Owned by:
Priority: major Milestone: sage-9.3
Component: linear algebra Keywords: matrix, subdivisions
Cc: chapoton Merged in:
Authors: Travis Scrimshaw Reviewers: Markus Wageringel
Report Upstream: N/A Work issues:
Branch: 65e036c (Commits, GitHub, GitLab) Commit: 65e036cd298f71b2db17302c391505d193956c22
Dependencies: Stopgaps:

Status badges

Description

sage: A = matrix.identity(QQ, 4)                                                
sage: A._subdivisions                                                           
sage: A2 = A.change_ring(RDF)                                                     
sage: A2._subdivisions                                                          
( [ 0, 4 ], [ 0, 4 ] )

We should also disregard when being passed no subdivisions:

sage: A = matrix.identity(QQ, 4)
sage: A._subdivisions
sage: A.subdivide(A.subdivisions())
sage: A._subdivisions
( [ 0, 4 ], [ 0, 4 ] )

Change History (13)

comment:1 Changed 8 months ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/matrix/unnecessary_subdivisions-31254
  • Commit set to c92d11fca6cbdcc339d32cb259acc617833bb205
  • Status changed from new to needs_review

This should take care of everything.


New commits:

c92d11fDo not perform any unnecessary subdivisions.

comment:2 Changed 8 months ago by gh-mwageringel

Oh, now I see that I had not fully understood the problem before, but I agree with the goal of this ticket now. This branch also fixes the problem in #31234, but nevertheless it still seems to be consistent to return a mutable matrix there. Sorry that I rushed it a bit.

comment:3 Changed 8 months ago by tscrim

This would be another fix to #31234, but I like the consistency there. Plus I see this as a slightly different issue.

comment:4 Changed 8 months ago by git

  • Commit changed from c92d11fca6cbdcc339d32cb259acc617833bb205 to f254535d02e8be194d655e561990fe2cd7cfa204

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

f254535Fixing one last doctest.

comment:5 Changed 8 months ago by tscrim

Here is a fix to the only doctest failure due to my standardization of the error message.

comment:6 Changed 8 months ago by tscrim

Green patchbot.

comment:7 Changed 8 months ago by gh-mwageringel

  • Reviewers set to Markus Wageringel

Thank you for solving this so quickly. This looks good to me – I just have a very small suggestion:

-        if row_lines == [] and col_lines == []:
+        if not row_lines and not col_lines:

comment:8 Changed 8 months ago by git

  • Commit changed from f254535d02e8be194d655e561990fe2cd7cfa204 to 65e036cd298f71b2db17302c391505d193956c22

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

65e036cDon't compare to the empty list.

comment:9 Changed 8 months ago by tscrim

Good point. I added parentheses around each to make it extra clear about the test. Doctests pass for me.

comment:10 Changed 8 months ago by tscrim

Green patchbot (style issues are unrelated to the changes here).

comment:11 Changed 8 months ago by gh-mwageringel

  • Status changed from needs_review to positive_review

comment:12 Changed 8 months ago by mkoeppe

  • Summary changed from Do not perform unnecesssary subdivisions in matrices to Do not perform unnecessary subdivisions in matrices

comment:13 Changed 8 months ago by vbraun

  • Branch changed from public/matrix/unnecessary_subdivisions-31254 to 65e036cd298f71b2db17302c391505d193956c22
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.