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: sage-8.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:

Status badges

Description

part of #16081

Change History (15)

comment:1 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/24249
  • Commit set to a1f694226e99cb17e6c531f35a1f7fa02cc0ce4b
  • Status changed from new to needs_review

New commits:

a1f6942py3: some minor care for range

comment:2 Changed 5 years ago by etn40ff

  • Reviewers set to Salvatore Stella
  • Status changed from needs_review to positive_review

LGTM

comment:3 Changed 5 years ago by jdemeyer

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

  • Commit changed from a1f694226e99cb17e6c531f35a1f7fa02cc0ce4b to 51207858a931dcbf65a668158bb257b0abdba8a2

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

5120785trac 24249 diagonal matrix of range

comment:5 Changed 5 years ago by jdemeyer

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 git

  • Commit changed from 51207858a931dcbf65a668158bb257b0abdba8a2 to 387df4378396540a1a22f1f9504e980a210cd805

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

387df43py3: some minor care for range

comment:7 Changed 5 years ago by chapoton

  • Status changed from needs_work to needs_review

I undid the change about diagonal matrices.

Last edited 5 years ago by chapoton (previous) (diff)

comment:8 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Same comment for giac.py: fix the code instead of the doctest.

comment:9 follow-up: Changed 5 years ago by chapoton

Really ? Do we really want to accept lists of range ?

comment:10 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

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 embray

The natural abstraction from lists would probably be to support any sized Collection.

comment:12 Changed 5 years ago by git

  • Commit changed from 387df4378396540a1a22f1f9504e980a210cd805 to 4642af336b3c640b30f0a099ae0772075354d7c2

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

4642af3correct one range, plus pep8 details

comment:13 Changed 5 years ago by chapoton

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

  • Status changed from needs_review to positive_review

Let's try again

comment:15 Changed 5 years ago by vbraun

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