Opened 9 years ago

Closed 5 years ago

#16082 closed enhancement (wontfix)

Python3 : Test ticket for zip()

Reported by: Wilfried Luebbe Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords: python3, days85
Cc: Erik Bray, Travis Scrimshaw, Jori Mäntysalo, Jeroen Demeyer, John Palmieri Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: public/16082 (Commits, GitHub, GitLab) Commit: 8795f70fbad9a3e3cbbdd329e071020062d0b5be
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

This ticket is used for doctesting the Python 3 compatibility of zip(). It is not a real ticket and should not be merged.

Change History (37)

comment:1 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:2 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:3 Changed 7 years ago by Jeroen Demeyer

Component: distributionpython3

comment:4 Changed 6 years ago by Frédéric Chapoton

Milestone: sage-6.4sage-7.4

comment:5 Changed 6 years ago by Frédéric Chapoton

one step in #21568

comment:6 Changed 6 years ago by Frédéric Chapoton

Milestone: sage-7.4sage-duplicate/invalid/wontfix
Status: newneeds_review

as of sage 7.5.rc2

running all doctests with "from future_builtins import zip" does not give any error

I propose therefore to close this ticket

EDIT: this needs to be checked very carefully.

Last edited 6 years ago by Frédéric Chapoton (previous) (diff)

comment:7 Changed 6 years ago by Julian Rüth

Could you elaborate on what you meant by "This needs to be checked very carefully."

comment:8 Changed 6 years ago by Frédéric Chapoton

Yes, I can.

What I mean is the following: one should find a way to run all doctests with the py3 behaviour of zip activated both in doctests and in code. I have not been able to do that so far, as it seems that adding an import line to src/sage/all.py is not enough.

Same thing for range in #16081

comment:9 Changed 6 years ago by Frédéric Chapoton

Branch: public/16082
Commit: 737c6682235caded1b7d809db84b8951016a4bbf

Here is a test branch, displaying the issue : how to trigger future zip behaviour in globally in the code ?


New commits:

737c668a test branch for zip, but not working as desired

comment:10 Changed 6 years ago by Julian Rüth

Could you patch globals()['__builtins__'].zip in sage.all?

comment:11 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewneeds_info

You should really think about putting from six.moves import zip in sage.all. That will affect everybody using Sage. Doing something like that should be explicitly discussed on sage-devel (and not in a thread called "how to check zip, map, range, print for Python3").

comment:12 in reply to:  11 ; Changed 6 years ago by Frédéric Chapoton

Replying to jdemeyer:

You should really think about putting from six.moves import zip in sage.all. That will affect everybody using Sage. Doing something like that should be explicitly discussed on sage-devel (and not in a thread called "how to check zip, map, range, print for Python3").

This is a *f#### test ticket*, with milestone wontfix. It has never been my intention to make this enter sage. Could you please make a more useful contribution ?

comment:13 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)
Status: needs_infoneeds_review
Summary: Python 3 preparation: The semantics of the zip() function is changedTest ticket for zip()

Was it really necessary to be so unpolite?

It seems that the only indication that it was a test ticket is the milestone. Nothing in the title nor the description nor in your sage-devel thread was making it clear that this was a test ticket. So that could easily be overlooked.

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:14 in reply to:  12 Changed 6 years ago by Jeroen Demeyer

Replying to chapoton:

Could you please make a more useful contribution ?

After being insulted like that? Why would I?

comment:15 Changed 6 years ago by Frédéric Chapoton

Sorry for my rough reaction. All my apologies. Early morning's bad mood.

Could you please, given all that you know that I don't, tell if you think that the goal of this ticket is reachable ?

comment:16 Changed 6 years ago by git

Commit: 737c6682235caded1b7d809db84b8951016a4bbf8795f70fbad9a3e3cbbdd329e071020062d0b5be

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

a843004Merge branch 'public/16082' in 7.6.b1
8795f70trac 16082 ! test branch for zip! do not merge !

comment:17 Changed 6 years ago by Frédéric Chapoton

ok, this works and discovers a lot of issues: two notable ones being

    for op,i in reversed(zip(ef, indices)):
    TypeError: argument to reversed() must be a sequence
return Factorization(zip(pols, exps), leading_coeff)
      File "/home/u1/chapoton/sage/local/lib/python2.7/site-packages/sage/structure/factorization.py", line 294, in __init__
        raise TypeError("x must be a list")

comment:18 Changed 6 years ago by Frédéric Chapoton

see #22271 for a partial cure

comment:19 Changed 6 years ago by Frédéric Chapoton

see #22350 for the next pill in the cure

comment:20 Changed 6 years ago by Frédéric Chapoton

then #22385

comment:21 Changed 6 years ago by Frédéric Chapoton

then #22457

comment:22 Changed 6 years ago by Frédéric Chapoton

then #22519

comment:23 Changed 6 years ago by Julian Rüth

Status: needs_reviewneeds_work

I find it always confusing that this "needs review". Can we leave it as "needs work" until all the zip problems are fixed?

comment:24 Changed 6 years ago by Frédéric Chapoton

They are now fixed, or should be. This really needs review now (but I admit it was set to needs review before reaching this state).

Last edited 6 years ago by Frédéric Chapoton (previous) (diff)

comment:25 Changed 6 years ago by Julian Rüth

Keywords: days85 added

comment:26 Changed 6 years ago by Julian Rüth

So is the question here now whether the patchbot is happy?

comment:27 Changed 6 years ago by Frédéric Chapoton

Yes, exactly. And sadly it seems not to be the case. At least this one failure seems related and needs investigation:

sage -t --long --warn-long 75.6 src/sage/graphs/generic_graph.py
**********************************************************************
File "src/sage/graphs/generic_graph.py", line 6558, in sage.graphs.generic_graph.GenericGraph.longest_path
Failed example:
    g.longest_path(algorithm="backtrack").edges()
Expected:
    [(0, 1, None), (1, 2, None), (2, 3, None), (3, 4, None), (4, 9, None), (5, 7, None), (5, 8, None), (6, 8, None), (6, 9, None)]
Got:
    []

comment:28 Changed 6 years ago by Frédéric Chapoton

see #22603 for maybe the last step

comment:29 Changed 6 years ago by Frédéric Chapoton

Status: needs_workneeds_review

green bot, which means that all zip problems have been taken care of

comment:30 Changed 6 years ago by Frédéric Chapoton

Summary: Test ticket for zip()Python3 : Test ticket for zip()

comment:31 Changed 6 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw Jori Mäntysalo Jeroen Demeyer John Palmieri added

can we please close this as duplicate, now that everything is in order ?

comment:32 Changed 6 years ago by Travis Scrimshaw

Do you want to keep this ticket around as a general test ticket against regressions or do you have another methodology for that?

comment:33 Changed 6 years ago by Frédéric Chapoton

I have no other methodology. Maybe it is better to keep it open so that patchbots can be launched on it. The patchbots should now be very reluctant to test closed tickets indeed.

comment:34 Changed 6 years ago by Frédéric Chapoton

Status: needs_reviewneeds_info

so let us rather put that into needs_info status

comment:35 Changed 5 years ago by Frédéric Chapoton

Status: needs_infopositive_review

I now propose to close this one. If somebody ever reintroduces a bad zip, we will see that when running all doctests with python3.

comment:36 Changed 5 years ago by Frédéric Chapoton

Cc: Erik Bray added

Erik, could you please close this one ?

comment:37 Changed 5 years ago by Erik Bray

Resolution: wontfix
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.