#26716 closed enhancement (fixed)

py3: some small care for matrix

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.5
Component: python3 Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer, Vincent Klein
Report Upstream: N/A Work issues:
Branch: 574f478 (Commits) Commit: 574f4789605096f8942ee818dd8a4e73b58b8fa3
Dependencies: Stopgaps:

Description


Change History (14)

comment:1 Changed 12 months ago by chapoton

  • Branch set to u/chapoton/26716
  • Status changed from new to needs_review

comment:2 Changed 12 months ago by git

  • Commit set to f8d780536b7fd34955a9b32743d0c4a287d5853d

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

f8d7805py3: some care for matrix

comment:3 Changed 12 months ago by git

  • Commit changed from f8d780536b7fd34955a9b32743d0c4a287d5853d to 6d94e11dbaa3fecfadcd6a30d4e6e9046506c61d

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

bb5184eMerge branch 'u/chapoton/26716' in 8.5.b4
6d94e11trac 26716 fix the pyflakes warnings

comment:4 Changed 12 months ago by vklein

  • Owner changed from (none) to vklein

comment:5 Changed 12 months ago by vklein

  • Owner changed from vklein to (none)

comment:6 Changed 12 months ago by jdemeyer

  1. This change looks suspicious, can you justify it:
    @@ -1927,6 +1928,7 @@ def block_matrix(*args, **kwds):
         sub_matrices = args[0]
     
         if is_Matrix(sub_matrices):
    +        M = sub_matrices
             # a single matrix (check nrows/ncols/ring)
             if (nrows is not None and nrows != 1) or \
                (ncols is not None and ncols != 1):
    

If it's a bug fix, then a doctest should be added.

  1. Replace if ring == QQ or ring == ZZ: by if ring is QQ or ring is ZZ:
  1. PEP 8 recommends to write
    entries = lambda i, j: 1 / (i + j + 1)
    

as

def entries(i, j):
    return 1 / (i + j + 1)

comment:7 Changed 12 months ago by git

  • Commit changed from 6d94e11dbaa3fecfadcd6a30d4e6e9046506c61d to 574f4789605096f8942ee818dd8a4e73b58b8fa3

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

574f478trac 26716 adding a missing doctest, plus details

comment:8 Changed 12 months ago by chapoton

About 1. Pyflakes said that M was not defined anywhere. And indeed, this part of the code was not doctested.

  1. and 3. Done

comment:9 Changed 12 months ago by chapoton

  • Cc tscrim added

ready for review and the bot is green

comment:10 Changed 12 months ago by vklein

  • Owner changed from (none) to vklein

comment:11 Changed 12 months ago by vklein

  • Owner changed from vklein to (none)

Looks good to me.

Side Note : This ticket is actually a bug fix with the M = sub_matrices modification.

Last edited 12 months ago by vklein (previous) (diff)

comment:12 Changed 12 months ago by vklein

  • Status changed from needs_review to positive_review

comment:13 Changed 12 months ago by vklein

  • Reviewers set to Jeroen Demeyer, Vincent Klein

comment:14 Changed 12 months ago by vbraun

  • Branch changed from u/chapoton/26716 to 574f4789605096f8942ee818dd8a4e73b58b8fa3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.