Upgrade to GAP 4.8.6

[slel]

GAP 4.8.6 was released on 2016-11-14.

Release announcement:
http://mail.gap-system.org/pipermail/forum/2016/005368.html

GAP 4.8.6 page
http://gap-system.org/Releases/4.8.6.html

Changelog:
http://www.gap-system.org/Manuals/doc/changes/chap2.html#X7B6C9EFD7DF68823

Tarballs:

• http://users.ox.ac.uk/~coml0531/sage/gap-4.8.6.tar.bz2
• http://users.ox.ac.uk/~coml0531/sage/database_gap-4.8.6.tar.bz2
• http://users.ox.ac.uk/~coml0531/sage/gap_packages-4.8.6.tar.bz2
• http://users.ox.ac.uk/~coml0531/sage/libgap-4.8.6.tar.gz

Depends on #22272

Upstream: Reported upstream. No feedback yet.

CC: @dimpase @kiwifb @vbraun @jdemeyer @tscrim

Component: group theory

Author: Dima Pasechnik

Branch/Commit: 8b8bff3

Reviewer: Volker Braun

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

[dimpase]

comment:2

Debian has patches for 4.8.6 update of libgap and gap, see
https://groups.google.com/d/msg/sage-packaging/O2PAsuAELMA/vKCdnu3RBAAJ

so we just should take them. Due to miscommunication with the Debian people, I only found out by chance, as I saw them asking what a GAP-related Sage doctest would output for GAP 4.8.6 in place of 4.8.3 :-)

[dimpase]

comment:3

a 1st try for update; already includes the patch from #22116, but still needs few doctest fixes, due to changes in random sources, mostly.

[dimpase]

Branch: u/dimpase/gap4.8.6

[dimpase]

Commit: 0244812

[sagetrac-git]

Changed commit from 0244812 to ba1e2ae

[sagetrac-git]

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

ba1e2ae

we don't have these packages installed by default

[dimpase]

Upstream: Reported upstream. No feedback yet.

[dimpase]

comment:5

libgap is built using modified upstream with manually adding src/libgap.map to the tarball as mentioned here. A proper (upstream) fix should be via automake config, I presume.

[dimpase]

Author: Dima Pasechnik

[sagetrac-git]

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

b6b44c7	GAPs random object generator has changed
2d86709	use improved GAPs Iterator and Enuerator in matrix_group.py
ce16e91	remaining easy doctest fixes

[sagetrac-git]

Changed commit from ba1e2ae to ce16e91

[jdemeyer]

comment:9

What do you expect from me? As I mentioned on #22116, I disagree with build/pkgs/gap_packages/patches/guava_leon.patch

[jdemeyer]

comment:10

I also wonder why you need the change to build/pkgs/database_gap/spkg-install. I don't know enough about GAP to know when gap_reset_workspace() is needed, but I think that you should give justification for that removal.

[dimpase]

comment:11

Replying to @jdemeyer:

What do you expect from me? As I mentioned on #22116, I disagree with build/pkgs/gap_packages/patches/guava_leon.patch

This is now upstream and will be in the next release of Guava.

[jdemeyer]

comment:12

Replying to @dimpase:

Replying to @jdemeyer:

What do you expect from me? As I mentioned on #22116, I disagree with build/pkgs/gap_packages/patches/guava_leon.patch

This is now upstream and will be in the next release of Guava.

Then please update the patch file with a pointer to upstream. For reference, also add it on this Trac page.

[dimpase]

comment:13

Replying to @jdemeyer:

I also wonder why you need the change to build/pkgs/database_gap/spkg-install. I don't know enough about GAP to know when gap_reset_workspace() is needed, but I think that you should give justification for that removal.

It is safe to omit this call. In fact,
gap_reset_workspace() needs some work. It has not really been touched in 5 years,
and it hangs on me now and then.
I do not want to do it on this ticket - it has been iffy for a while already.

[sagetrac-git]

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

990b5db	added a pointer to upstream change
35c8fea	workspace will be updated automatically anyway.

[sagetrac-git]

Changed commit from ce16e91 to 35c8fea

[tscrim]

comment:35

Replying to @dimpase:

Replying to @jhpalmieri:

With 7.6.beta2, this works:

sage: W = WeylGroup(["A",3,1])
sage: for w in W: print w
   (... neverending list ...)

I believe this invokes W.__iter__, right?
And the latter is in sage/groups/libgap_mixin.py (cf. W.__iter__??)
So this is merely calling self.gap().Iterator(). And the latter does not actually work in Sage 7.5; one gets

ValueError: libGAP: Error, no method found! Error, no 3rd choice method found for `Enumerator' on 1 arguments

whereas with the branch here it does "work" - it runs out of memory...

I would suspect this is a GAP problem by trying to put the entire (matrix) group into memory as a list under an assumption that it is a finite group. Well, our RecursiveEnumeratedSet iterator runs into the same problem, but it will at least output things along the way...

That is, I believe that the category framework (or something like this) does try to call that W.__iter__, and upon failing (with the old GAP) actually does something that makes sense, namely sage/groups/matrix_gps/matrix_group.py has the lines

if not self.is_finite():
    # use implementation from category framework
    for g in super(Group, self).__iter__():
          yield g
    return

where the code in sage/categories/coxeter_group.py is called to do the right thing.

It's actually the reverse that happens: code in the categories is the last called in the MRO. In particular, the first call is to the lines copied above, and for non-finite groups (or at least those not known to be finite), then call up to the corresponding __iter__ implemented in the category (if there is one).

See comment:29 for why it used to work for Weyl groups.

Whereas with the new GAP one does not get a ValueError and proceeds with running out of memory (actual running done by GAP).

You deleted the code above that would call up to the category framework, where it was not using GAP to do the iteration.

So, somewhere, something must forbid this execution path, but I don't know where and how. All help is welcome. If the fix is not going to happen in GAP-related
code, I'd be inclined to press ahead with this ticket, declaring this issue a known (Sage) bug.

You probably need to restore part of the __iter__, specifically the code quoted above.

TL;DR - As John said, the problem is removing __iter__ from sage/groups/matrix_groups/matrix_group.py.

[dimpase]

comment:36

Oh, I see. Thanks. Sorry for taking so long to get it. I removed too mich in this commit. I will reinstate the part that deals with infinite groups.

However this is still less than ideal. In the finite case for groups that have a fast category framework iterator (e.g. large finite Coxeter groups) one should not call GAP, either. Should this be dealt with on a follow-up ticket, or is there a quick way to add the corresponding filter?

[sagetrac-git]

Changed commit from d861b37 to 971f323

[sagetrac-git]

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

971f323

reintroduce `__init__` in MatrixGroup_gap to handle infinite groups

[sagetrac-git]

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

fd68158

putting the doctests right

[sagetrac-git]

Changed commit from 971f323 to fd68158

[tscrim]

comment:39

Replying to @dimpase:

However this is still less than ideal. In the finite case for groups that have a fast category framework iterator (e.g. large finite Coxeter groups) one should not call GAP, either. Should this be dealt with on a follow-up ticket, or is there a quick way to add the corresponding filter?

The category iterator is not just for Coxeter groups, but any group with a finite generating set. Have you run timings (and memory usage) comparing the GAP iterator with the one from the category?

At least for Coxeter groups, we can do a check on the rank if there is some cutoff point.

[dimpase]

Changed work issues from failing long doctests to none

[vbraun]

Reviewer: Volker Braun

[vbraun]

comment:42

Testsuite fails:

########> Diff in /Users/buildslave-sage/slave/sage_git/build/local/gap/latest\
1045/tst/testinstall/grpmat.tst, line 32:
1046# Input is:
1047Size( AutomorphismGroup( G ) );
1048# Expected output:
104924
1050# But found:
1051#I  autpgrp package is not available. Check that the name is correct
1052#I  and it is present in one of the GAP root directories (see '??RootPaths')

[dimpase]

comment:43

Replying to @vbraun:

Testsuite fails:

########> Diff in /Users/buildslave-sage/slave/sage_git/build/local/gap/latest\
1045/tst/testinstall/grpmat.tst, line 32:
...
1051#I  autpgrp package is not available. Check that the name is correct
1052#I  and it is present in one of the GAP root directories (see '??RootPaths')

As far as I am concerned, this is not a bug, but a decision we made at some point to ship only the very minimal default GAP configuration.
As you most probably know, GAP assumes quite a few packages to be installed and autoloaded by default; this is patched away by
​ba1e2ae "we don't have these packages installed by default"

Sage's GAP does not install autpgrp by default, and does not autoload it if it is available. Suppose it is available (i.e. gap_packages is installed); then
before running GAP_ROOT/tst/testinstall.g one has to load autpgrp manually.
I.e.

gap> LoadPackage("autpgrp");;
gap> Read("testinstall.g");

will make everything pass (on my system at least).

[vbraun]

comment:44

Still I'd prefer to have a passing testsuite so that the buildbots can actuall run the tests. Can we just grep out tst/testinstall/grpmat.tst in spkg-check?

[vbraun]

comment:45

I guess it is annoying because of the newline; How about deleting grpmat.tst from the install then?

[sagetrac-git]

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

8b8bff3

fake the output of GAP test needing autpgrp pkg

[sagetrac-git]

Changed commit from fd68158 to 8b8bff3

[dimpase]

comment:47

Replying to @sagetrac-git:

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

8b8bff3

fake the output of GAP test needing autpgrp pkg

this should do the trick - I just fake the output of the test in question.

[vbraun]

Changed branch from u/dimpase/gap4.8.6 to 8b8bff3