Opened 5 years ago
Closed 5 years ago
#24249 closed enhancement (fixed)
py3: minor care for range
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Salvatore Stella 
Report Upstream:  N/A  Work issues:  
Branch:  4642af3 (Commits, GitHub, GitLab)  Commit:  4642af336b3c640b30f0a099ae0772075354d7c2 
Dependencies:  Stopgaps: 
Description
part of #16081
Change History (15)
comment:1 Changed 5 years ago by
 Branch set to u/chapoton/24249
 Commit set to a1f694226e99cb17e6c531f35a1f7fa02cc0ce4b
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Reviewers set to Salvatore Stella
 Status changed from needs_review to positive_review
LGTM
comment:3 Changed 5 years ago by
 Status changed from positive_review to needs_work
Sorry, but I have to disagree here.
As I have mentioned several times before, you should really avoid "fixing" doctests. For example, matrix.diagonal(range(n))
really should work. If it doesn't work, then fix matrix.diagonal()
to accept iterables instead of "fixing" the doctest. Fixing the doctests will actually make it harder to port to Python 3 because it just hides issues.
comment:4 Changed 5 years ago by
 Commit changed from a1f694226e99cb17e6c531f35a1f7fa02cc0ce4b to 51207858a931dcbf65a668158bb257b0abdba8a2
Branch pushed to git repo; I updated commit sha1. New commits:
5120785  trac 24249 diagonal matrix of range

comment:5 Changed 5 years ago by
Can we please have a separate ticket for the issues of diagonal_matrix(range(n))
and matrix([range(n)])
? Stuffing this in a Python 3 ticket isn't practical for the discussion.
comment:6 Changed 5 years ago by
 Commit changed from 51207858a931dcbf65a668158bb257b0abdba8a2 to 387df4378396540a1a22f1f9504e980a210cd805
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
387df43  py3: some minor care for range

comment:7 Changed 5 years ago by
 Status changed from needs_work to needs_review
I undid the change about diagonal matrices.
comment:8 Changed 5 years ago by
 Status changed from needs_review to needs_work
Same comment for giac.py
: fix the code instead of the doctest.
comment:9 followup: ↓ 10 Changed 5 years ago by
Really ? Do we really want to accept lists of range ?
comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to chapoton:
Really ? Do we really want to accept lists of range ?
Of course, why not? It's the obvious generalization of a list of lists.
comment:11 Changed 5 years ago by
The natural abstraction from lists would probably be to support any sized Collection.
comment:12 Changed 5 years ago by
 Commit changed from 387df4378396540a1a22f1f9504e980a210cd805 to 4642af336b3c640b30f0a099ae0772075354d7c2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4642af3  correct one range, plus pep8 details

comment:13 Changed 5 years ago by
 Status changed from needs_work to needs_review
ok, I undid also the other change in giac.
now, this contains only one correction to range "in the code", plus a pep8 cleanup of the onlyu modified file.
comment:14 Changed 5 years ago by
 Status changed from needs_review to positive_review
Let's try again
comment:15 Changed 5 years ago by
 Branch changed from u/chapoton/24249 to 4642af336b3c640b30f0a099ae0772075354d7c2
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: some minor care for range