py3: get rid of some filter in classical geometries

[fchapoton]

Depends on #24460

CC: @dimpase

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 227760d

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/24984

[fchapoton]

Branch: u/chapoton/24984

[fchapoton]

Commit: 7d97127

[fchapoton]

New commits:

7d97127

get rid of some filter(...) in classical geometries

[fchapoton]

comment:2

This should also be fixed:

    if clique_partition:
        lines = map(lambda x: filter(lambda t: t[0]+x*t[1]==0, V),
                     filter(lambda z: z != 0, Fq))
        return (G, lines, v0)

Dima, could you please take care of that ?

[jdemeyer]

comment:3

Code like

        V = [x for x in libgap.Orbits(g, S, libgap.OnLines) if len(x) == nvert]
        assert len(V) == 1
        V = V[0]

can better be written as

        (V,) = [x for x in libgap.Orbits(g, S, libgap.OnLines) if len(x) == nvert]

[sagetrac-git]

Changed commit from 7d97127 to 0e5bc56

[sagetrac-git]

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

0e5bc56

trac 24984 details

[fchapoton]

comment:5

ok, done

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:6

positive review if tests pass

[dimpase]

comment:7

Replying to @jdemeyer:

Code like

        V = [x for x in libgap.Orbits(g, S, libgap.OnLines) if len(x) == nvert]
        assert len(V) == 1
        V = V[0]

can better be written as

        (V,) = [x for x in libgap.Orbits(g, S, libgap.OnLines) if len(x) == nvert]

Sorry, how is this equivalent? The assert there means to be an extra guard against a SNAFU, and now it's gone.

[fchapoton]

comment:8

the assignement will protest if the list has not the correct expected length, here 1

try

(a,) = (4,5)

[dimpase]

comment:9

By the way, what's wrong with filter() in the 1st place?

[fchapoton]

comment:10

problem in py3, filter is an iterator, and you cannot ask for V[0]

[fchapoton]

comment:11

by the way, did you manage to get a python3-sage up and running ?

[dimpase]

comment:12

Replying to @fchapoton:

problem in py3, filter is an iterator, and you cannot ask for V[0]

and where does one ask for V[0]?

I'd rather apply list() to an iterator; e.g. wouldn't this (from comment 2) work in python3?

    if clique_partition:
        lines = map(lambda x: filter(lambda t: t[0]+x*t[1]==0, V),
                     filter(lambda z: z != 0, Fq))
        return (G, list(lines), v0)

---

No, I'm not python3ised yet, sorry :-)

[dimpase]

comment:13

Replying to @fchapoton:

the assignement will protest if the list has not the correct expected length, here 1

try

(a,) = (4,5)

Right, this makes sense, thanks.

[fchapoton]

comment:14

after the branch here, one does not longer ask for V[0]. Before, one did

In principle, vanilla-sage 8.2.b8 should build and start with python3. Instructions:
(in a separate clone of the git repo)

make configure
./configure --with-python=3
make build

Give it a try of you can.

[dimpase]

comment:15

this does not look like a right change

-        lines = map(lambda x: filter(lambda t: t[0]+x*t[1]==0, V),
-                     filter(lambda z: z != 0, Fq))
+        lines = [[t for t in V if t[0] + z * t[1] == 0]
+                 for z in Fq if z]

x from V is gone and replaced by z from Fq in t[0]+x*t[1]==0.

This is why I'd rather retain all the filters, you know...

[fchapoton]

comment:16

well, I did a mistake, please correct it, and let us try to spend only a short time on the ticket, please..

[dimpase]

comment:17

Would you mind if I try installing sage@py3 and provide a branch I don't have to wrap my poor brain around unnecessarily?

Sorry, I am badly jetlagged and still somewhere between Japanese time an GMT...

[fchapoton]

comment:18

This ticket has no urgency, and I like the branch as it is. Please take a rest, a nap, or a good night sleep. We will look at that later.

EDIT: I have double-checked my changes after if clique_partition:, and I think that they are correct.

[dimpase]

comment:19

before working on this, we ought to remedy the root cause of most doctest failures there, namely the fact that libgap is broken. Indeed:

sage: libgap.GeneralUnitaryGroup(4, 2)
------------------------------------------------------------------
TypeError                        Traceback (most recent call last)
<ipython-input-1-26e9bcf1ef0f> in <module>()
----> 1 libgap.GeneralUnitaryGroup(Integer(4), Integer(2))

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__getattr__ (build/cythonized/sage/misc/lazy_import.c:3558)()

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.get_object (build/cythonized/sage/misc/lazy_import.c:2210)()

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport._get_object (build/cythonized/sage/misc/lazy_import.c:2480)()

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in init sage.libs.gap.libgap()
    785 
    786 
--> 787 libgap = Gap()

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.__init__ (build/cythonized/sage/libs/gap/libgap.c:5511)()
    615             <class 'sage.libs.gap.libgap.Gap'>
    616         """
--> 617         initialize()
    618         libgap_set_gasman_callback(gasman_callback)
    619         from sage.rings.integer_ring import ZZ

/mnt/opt/Sage/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.initialize (build/cythonized/sage/libs/gap/util.c:4459)()

TypeError: expected bytes, str found

It's really baffling kind of errors message...

---

I was able to build sage@py3 capable of doctesting, following comment 14, with added #24343 and #24922.

[fchapoton]

comment:20

Well, there are maybe thousands of issues remaining, some small and some big. Unicode problems are a large part of that. If we could fix the 3 tiny problems here quickly, I would prefer.

Bot is morally green.

[dimpase]

Dependencies: #24990

[dimpase]

comment:21

I believe we should first be able to test that these fixes actually work on py3, too, hence the dependence. Once libgap works, this should be a breeze to fix everything py3-related here.

Moreover I actually like the code using filters, as it is using iterators (on py3) rather than explicit lists, which might lead to various performance gains.
So I am not ready to just give it up, especially as trivial fixes like applying list() to an iterator are always possible and are much less intrusive.

[fchapoton]

comment:22

I do not believe we should wait for any other ticket. Please consider seriously the awful amount of remaining work to get sage fully py3-compatible, and accept to let it go. I promise I will not try to remove filter everywhere else.

[dimpase]

comment:23

I am sorry, but indiscriminate replacing of iterators with eager iterable data structures is code vandalism, as it potentially degrades performance, and does more harm than good.

Once the ability to properly test the code arrives, it becomes very easy.

[dimpase]

Changed dependencies from #24990 to #24460

[jdemeyer]

comment:25

I'll leave you to it.

[jdemeyer]

Changed reviewer from Jeroen Demeyer to none

[sagetrac-git]

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

3a73b04	Merge branch 'u/chapoton/24984' in 8.2
227760d	make the pyflakes plugin happy

[sagetrac-git]

Changed commit from 0e5bc56 to 227760d

[embray]

comment:28

Replying to @dimpase:

I am sorry, but indiscriminate replacing of iterators with eager iterable data structures is code vandalism, as it potentially degrades performance, and does more harm than good.

It's not "code vandalism". Currently the changes Frédéric has made here is just replacing some code with the exact equivalent (on Python 2) code that also works on Python 3. If you believe these algorithms can be improved by the use of iterators you're welcome to take a stab at it, but as currently written that is not the case. I can certainly see a couple small opportunities here, but it is not a priority as there is no regression here--the higher priority right now is getting Python 3 working.

[embray]

Reviewer: Erik Bray

[vbraun]

Changed branch from u/chapoton/24984 to 227760d