Upgrade of p_group_cohomology spkg

[simon-king-jena]

In the past, p_group_cohomology-2.1.4 was an optional spkg. Sage dropped support for old style spkgs, and thus only an experimental spkg remained.

The purpose of this ticket is to provide a new group cohomology package for Sage. I suggest to make it optional (not experimental).

In the experimental version 2.1.5, I have improved the computation of Poincaré series, depth and filter degree type (the latter now uses a Hilbert-driven computation of Gröbner bases in elimination order, which works since in that setting the Hilbert function is easily available), and I added new functionality related with nilpotency.

Package structure

Version 3.0 is refactored as follows:

• It depends on the optional meataxe package (see Turn MeatAxe into a dynamic library #24359), which is based on a fork of MeatAxe.
• The spkg-install script first tests whether the SmallGroups library is installed in GAP. If it isn't, a warning is printed; however, the package still gets installed, as it only is a runtime dependency.
• The C code written by David Green has become an autotoolized shared library.
• Gap and Singular code is put into SAGE_SHARE/, where GAP and Singular libraries belong.
• The rest of the package is formed by python and cython code, and uses pip for installation.
• The documentation is built in an improved way.

Further changes

• The custom logging in the old spkg is replaced by Python's logging module.
• The tests avoid side-effects by using a reset function. Hence, no custom doc tester is needed: sage -t is enough.
• In the previous spkg, I tried to be as backward compatible as possible. But now I'm dropping support for very old Singular versions.
• Some experimental options for the cohomology computation are removed.

Many tests have changed, since the ring presentations of the computed cohomology rings, specifically for non-primepower groups, have changed. I do not fully understand why they changed. but it seems that it is a consequence of changes in Singular. The changes did (of course...) not affect the isomorphism types of the rings.

Upstream tar ball

https://users.fmi.uni-jena.de/cohomology/p_group_cohomology-3.0.tar.xz

Upstream: None of the above - read trac for reasoning.

CC: @jhpalmieri @vbraun @jdemeyer @haraldschilly @frederichan-IMJPRG [email protected]

Component: packages: optional

Keywords: group cohomology

Author: Simon King

Branch: 6576915

Reviewer: Travis Scrimshaw, Jeroen Demeyer

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

[simon-king-jena]

comment:1

For me, all but two tests fail. Both tests fail with "connection timed out" during access to a web data base, and both work fine when done in an interactive session.

Has something in the test framework changed, so that web access via urllib2 fails during tests?

[jdemeyer]

comment:2

I see you are compiling some extensions with the csage library but there is a ticket (#17854) which will make that library disappear. Ideally, you should only compile against csage only if it actually exists.

[simon-king-jena]

comment:3

Replying to @jdemeyer:

I see you are compiling some extensions with the csage library but there is a ticket (#17854) which will make that library disappear. Ideally, you should only compile against csage only if it actually exists.

OK. So I should see what I actually use of csage.

[jdemeyer]

comment:4

The sig_on() framework was in csage until 6.7 and got moved to the Sage library in 6.8.beta0 (#18027). So you can probably just link against csage if the Sage version is <= 6.7

[simon-king-jena]

comment:5

Replying to @jdemeyer:

The sig_on() framework was in csage until 6.7 and got moved to the Sage library in 6.8.beta0 (#18027). So you can probably just link against csage if the Sage version is <= 6.7

OK. What does that mean for the code? Do I need to import anything if I am using sig_on/off, or would this just work automatically with the new Cython version?

[jdemeyer]

comment:6

Replying to @simon-king-jena:

OK. What does that mean for the code? Do I need to import anything if I am using sig_on/off, or would this just work automatically with the new Cython version?

You just include sage/ext/interrupt.pxi, that doesn't change.

The main difference is that you no longer need to add csage as library. Currently, it doesn't hurt if you add the library anyway, but in the future, csage might not exist anymore.

And there is one additional caveat which is relevant for your package: since #18027, you must include interrupt.pxi only in .pyx files and not .pxd files (unless you really know what you're doing).

[jdemeyer]

comment:7

On the topic of .pxd files, this is what they should contain:

1. The API of your module, that is all cdef classes and c(p)def functions that you want to export for use in other modules (in the same package or by other packages).

2. If your module provides bindings to some library (the mtx module in your package for example), then add the declarations of the library functions in the .pxd file.

3. Any dependencies of the above. Sage example: if you're defining a new kind of element of a ring, you will probably inherit from RingElement. In your .pxd file, you want to write cdef class MySpecialElement(RingElement). This makes RingElement a dependency, so you need to write from ... cimport RingElement in the .pxd file.

And that's it really. Things like interrupts have nothing to do with the API of your module, those are implementation details and belong in the .pyx file. This is important in order to minimize dependencies.

NOTE about 2: if your module provides bindings to some library but also does significant other stuff, then it's best to split up the bindings in a different .pxd file. In Sage, we usually put the library bindings in sage/libs, look at sage/libs/gmp for a good example.

[simon-king-jena]

comment:8

Replying to @jdemeyer:

On the topic of .pxd files, this is what they should contain:

1. The API of your module, that is all cdef classes and c(p)def functions that you want to export for use in other modules (in the same package or by other packages).

2. If your module provides bindings to some library (the mtx module in your package for example), then add the declarations of the library functions in the .pxd file.

3. Any dependencies of the above. Sage example: if you're defining a new kind of element of a ring, you will probably inherit from RingElement. In your .pxd file, you want to write cdef class MySpecialElement(RingElement). This makes RingElement a dependency, so you need to write from ... cimport RingElement in the .pxd file.

And that's it really. Things like interrupts have nothing to do with the API of your module, those are implementation details and belong in the .pyx file. This is important in order to minimize dependencies.

OK. That's something that I should change. Actually, until not so long ago, I thought that cimports were only possible in pxd files.

NOTE about 2: if your module provides bindings to some library but also does significant other stuff, then it's best to split up the bindings in a different .pxd file. In Sage, we usually put the library bindings in sage/libs, look at sage/libs/gmp for a good example.

Do I understand correctly: You suggest that I should create a new pxd file for the MeatAxe bindings, say, mtx_lib.pxd, which is then cimported in mtx.pxd?

[jdemeyer]

comment:9

Replying to @simon-king-jena:

Do I understand correctly: You suggest that I should create a new pxd file for the MeatAxe bindings, say, mtx_lib.pxd, which is then cimported in mtx.pxd?

I cannot really speak about this specific case, but it certainly would make sense to split it up.

[simon-king-jena]

Work Issues: Cope with yet another backward incompatible change of Sage

[simon-king-jena]

comment:11

I just found that the package won't build with the latest beta of Sage. Reason:

pGroupCohomology/cohomology.c:248:36: fatal error: sage/ext/interrupt/pxi.h: No such file or directory
 #include "sage/ext/interrupt/pxi.h"
                                    ^
compilation terminated.
error: command 'gcc' failed with exit status 1

So, can you please tell me how to use interrupts, after all these backward incompatible changes? It really sucks.

[simon-king-jena]

comment:12

Good news! On my machine, I did some refactoring of code according to the hints you gave me---and that did the trick.

My plan is to continue refactoring, taking into account what you said about where to import stuff, and then push a new package version to the official page.

[simon-king-jena]

comment:13

Bad news. It builds, but I can't import it. Which brings up the question again why there are all these incompatible changes in Sage. They didn't use to occur so frequently in earlier versions of Sage, IIRC.

[simon-king-jena]

comment:14

I got it fixed. So, you'll find the updated version of p_group_cohomology-2.1.5 on my web page.

Some of your concerns about importing/including stuff are addressed in the new version: I moved some of it from .pxd files into .pyx files. What I did not do is to split off library bindings and put them into separate .pxd files. That would still make sense, but for now I put the ticket back to "needs review".

[simon-king-jena]

Changed work issues from Cope with yet another backward incompatible change of Sage to none

[simon-king-jena]

comment:15

Oh, and I also created a new module pGroupCohomology.auxiliaries, in which I've put some auxiliary functions that have previously been in pGroupCohomology.resolution.

[frederichan-IMJPRG]

comment:16

I have a built failure for
sage -i upstream/p_group_cohomology-2.1.5.spkg
with sage 6.8beta3+trac18666 (but success with master)

gcc -pthread -shared -L/home/fred/dev/sage/local/lib build/temp.linux-x86_64-2.7/pGroupCohomology/resolution.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/aufloesung.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/aufnahme.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/djgerr.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/fileplus.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/nBuchberger.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/pgroup.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/pincl.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/slice.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/urbild.o build/temp.linux-x86_64-2.7/pGroupCohomology/c_sources/uvr.o -L/home/fred/dev/sage/local/lib -lmtx -lpython2.7 -o build/lib.linux-x86_64-2.7/pGroupCohomology/resolution.so
cythoning pGroupCohomology/cohomology.pyx to pGroupCohomology/cohomology.c
building 'pGroupCohomology.cohomology' extension
gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/fred/dev/sage/src/c_lib/include -I/home/fred/dev/sage/local/share/sage/ext -I/home/fred/dev/sage/src/sage/ext -Imtx2.2.4/src -IpGroupCohomology/c_sources -IpGroupCohomology -I/home/fred/dev/sage/local/include/python2.7 -c pGroupCohomology/cohomology.c -o build/temp.linux-x86_64-2.7/pGroupCohomology/cohomology.o -O3
In file included from pGroupCohomology/cohomology.c:247:0:
mtx2.2.4/src/meataxe.h:15:0: warning: "_GNU_SOURCE" redefined
 #define _GNU_SOURCE
 ^
In file included from pGroupCohomology/cohomology.c:8:0:
/home/fred/dev/sage/local/include/python2.7/pyconfig.h:1160:0: note: this is the location of the previous definition
 #define _GNU_SOURCE 1
 ^
pGroupCohomology/cohomology.c:262:37: fatal error: sage/libs/ntl/ntlwrap.cpp: No such file or directory
 #include "sage/libs/ntl/ntlwrap.cpp"
                                     ^
compilation terminated.
error: command 'gcc' failed with exit status 1
Error installing Cython code.

real	0m43.412s
user	0m40.636s
sys	0m2.736s
************************************************************************
Error installing package p_group_cohomology-2.1.5
************************************************************************

[jdemeyer]

comment:17

You might want to wait for #18494, which should make it easier to install external packages using the Sage library.

[simon-king-jena]

comment:18

The real questions are: Why is it looking for ntl? I don't think it is explicitly requested by my package, thus, it probably is an indirect dependency, i.e., one that is in fact a dependency of something in Sage that I cimport into my package. But if it is a dependency of something in Sage, then why does it not work?

I suppose that I have to provide an include path to fix it. If the dependency really is via something in the Sage library, then this "have to" is not very nice---Sage could figure it out without human help.

[jdemeyer]

comment:19

Replying to @simon-king-jena:

The real questions are: Why is it looking for ntl?

Because the Sage <-> NTL interface is a real mess. In particular, any module which needs just a fragment of NTL automatically cimports essentially all of the NTL interface. In particular, sage.rings.integer does that.

[jdemeyer]

comment:20

In setup.py, replace

              include_dirs = CSAGE_PATH + [
                              os.path.join(SAGE_SRC,"c_lib","include"),
                              SAGE_EXTCODE, SRC_EXT,
                              os.path.join("mtx2.2.4","src"),
                              os.path.join("pGroupCohomology","c_sources"),
                              "pGroupCohomology"]

by

try:
    # Sage >= 6.8
    from sage.env import sage_include_directories
except ImportError:
    # Sage < 6.8
    def sage_include_directories():
        return [
            os.path.join(SAGE_LOCAL, "include", "csage"),
            os.path.join(SAGE_SRC, "c_lib", "include"),
            SAGE_EXTCODE, SRC_EXT]  # Not sure that you really need SAGE_EXTCODE

...

              include_dirs = sage_include_directories() + [
                             os.path.join("mtx2.2.4","src"),
                             os.path.join("pGroupCohomology","c_sources"),
                             "pGroupCohomology"]

and use #18494.

[jdemeyer]

comment:554

There is an undeclared dependency on matplotlib: the testsuite plots stuff, so you should add matplotlib as dependency (order-only, so after |).

[sagetrac-git]

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

4b89060	p_group_cohomology-3.0: Add matplotlib as dependency
45146d8	p_group_cohomology-3.0: Add sage.tests.modular_group_cohomology

[sagetrac-git]

Changed commit from 0aaeef3 to 45146d8

[simon-king-jena]

comment:556

Replying to @jdemeyer:

There is an undeclared dependency on matplotlib: the testsuite plots stuff, so you should add matplotlib as dependency (order-only, so after |).

Is it not part of $SAGE_RUNTIME? In any case, I added it as a dependency.

Also, I added src/sage/tests/modular_group_cohomology.py. The tests there take 9 seconds (I hope that's OK), and they cover a wide range of functionality of the package. Hence, if some change in the sage library breaks p_group_cohomology and a user who has installed the package runs make test, then the problem is very likely to become visible immediately.

I guess it is back to "needs review", right?

[jdemeyer]

comment:557

OK, let me test this new version on a few machines. If everything goes well, I agree that positive review can be given.

[tscrim]

comment:558

So on my machine, I get

sage -t --warn-long src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 46, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H.essential_ideal()                           # optional - p_group_cohomology
Test ran for 1.39 s
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 62, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H = CohomologyRing(gap(AlternatingGroup(7)), GroupName="A(7)", prime=2, from_scratch=True) # optional - p_group_cohomology
Test ran for 1.51 s
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 79, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H = CohomologyRing(gap(AlternatingGroup(6)), GroupName="A(6)", prime=3, from_scratch=True) # optional - p_group_cohomology
Test ran for 1.90 s

So while these tests are longer than 1 second on my machine, I think they are not sufficiently long to warrant a # long time considering the purpose of this file.

[simon-king-jena]

comment:559

Replying to @tscrim:

So on my machine, I get

sage -t --warn-long src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 46, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H.essential_ideal()                           # optional - p_group_cohomology
Test ran for 1.39 s
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 62, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H = CohomologyRing(gap(AlternatingGroup(7)), GroupName="A(7)", prime=2, from_scratch=True) # optional - p_group_cohomology
Test ran for 1.51 s
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 79, in sage.tests.modular_group_cohomology
Warning, slow doctest:
    H = CohomologyRing(gap(AlternatingGroup(6)), GroupName="A(6)", prime=3, from_scratch=True) # optional - p_group_cohomology
Test ran for 1.90 s

Would one get these warnings automatically? Or does one need to provide a special option? The complete log for my test run is this:

$ ./sage -t src/sage/tests/modular_group_cohomology.py 
too many failed tests, not using stored timings
Running doctests with ID 2018-06-30-20-33-28-f134a034.
Git branch: t/18514/upgrade_of_group_cohomology_spkg
Using --optional=database_gap,meataxe,mpir,p_group_cohomology,python2,sage
Doctesting 1 file.
sage -t src/sage/tests/modular_group_cohomology.py
    [16 tests, 9.26 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 9.3 seconds
    cpu time: 7.3 seconds
    cumulative wall time: 9.3 seconds

It is of course conceivable that some of the 16 tests actually take longer than one second (given that on average they take 0.58 seconds). But has the policy changed? I seem to remember that when I was young, tests were supposed to be "not longer than a few seconds". But just one second seems short to me.

[simon-king-jena]

comment:560

Replying to @simon-king-jena:

Would one get these warnings automatically? Or does one need to provide a special option?

Sorry, I didn't notice that you used sage -t --warn-long.

On my machine, I get warnings for four tests:

    H.essential_ideal()
    ascii_art(H.bar_code('LowerCentralSeries')[2])
    H = CohomologyRing(gap(AlternatingGroup(7)), GroupName="A(7)", prime=2, from_scratch=True)
    H = CohomologyRing(gap(AlternatingGroup(6)), GroupName="A(6)", prime=3, from_scratch=True)

I'll try to come up with shorter tests.

[sagetrac-git]

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

d4a612c

p_group_cohomology-3.0: Simplify tests/modular_group_cohomology.py

[sagetrac-git]

Changed commit from 45146d8 to d4a612c

[simon-king-jena]

comment:562

I have simplified the tests and still think that they cover a wide range of functionality. This time I didn't get a warning (and I tested 10 times):

$ ./sage -t --warn-long src/sage/tests/modular_group_cohomology.py
Running doctests with ID 2018-07-01-00-21-35-4bcb0f8f.
Git branch: t/18514/upgrade_of_group_cohomology_spkg
Using --optional=database_gap,meataxe,mpir,p_group_cohomology,python2,sage
Doctesting 1 file.
sage -t --warn-long src/sage/tests/modular_group_cohomology.py
    [17 tests, 5.15 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 5.2 seconds
    cpu time: 4.0 seconds
    cumulative wall time: 5.2 seconds

[jdemeyer]

comment:563

Replying to @tscrim:

So while these tests are longer than 1 second on my machine, I think they are not sufficiently long to warrant a # long time considering the purpose of this file.

Why not? The official policy is that normal tests should take less than 1 second, # long time tests less than 30 seconds. Remember that 1 second on your machine may be multiple seconds on a slower machine.

[tscrim]

comment:564

Replying to @jdemeyer:

Replying to @tscrim:

So while these tests are longer than 1 second on my machine, I think they are not sufficiently long to warrant a # long time considering the purpose of this file.

Why not? The official policy is that normal tests should take less than 1 second, # long time tests less than 30 seconds. Remember that 1 second on your machine may be multiple seconds on a slower machine.

2/3 were less than 1.5 seconds, and my machine is not particularly fast. I am happier to have the shorter tests. Yet, I think this is something that will be tested only by the bots (if they even have this optional package installed) and is meant to catch interface breakage, I did not see as much harm in having longer regular tests.

[jdemeyer]

Changed branch from u/SimonKing/upgrade_of_group_cohomology_spkg to u/jdemeyer/upgrade_of_group_cohomology_spkg

[jdemeyer]

Reviewer: Travis Scrimshaw, Jeroen Demeyer

[jdemeyer]

comment:566

Replying to @tscrim:

2/3 were less than 1.5 seconds, and my machine is not particularly fast. I am happier to have the shorter tests. Yet, I think this is something that will be tested only by the bots (if they even have this optional package installed) and is meant to catch interface breakage, I did not see as much harm in having longer regular tests.

On the other hand, those bots typically run tests with --long, so it wouldn't hurt to add # long time in that case.

I just added a few trivial changes to wrap some long lines, so I guess this is finally good to go...

---

New commits:

10cdf0e	Add optional package p_group_cohomology-3.0
6576915	Wrapping some long lines

[jdemeyer]

Changed commit from d4a612c to 6576915

[simon-king-jena]

comment:567

Replying to @jdemeyer:

Replying to @tscrim:

2/3 were less than 1.5 seconds, and my machine is not particularly fast. I am happier to have the shorter tests. Yet, I think this is something that will be tested only by the bots (if they even have this optional package installed) and is meant to catch interface breakage, I did not see as much harm in having longer regular tests.

On the other hand, those bots typically run tests with --long, so it wouldn't hurt to add # long time in that case.

Just for the record: The tests in the final version are all less than a second.

In any case: Thank you for the positive review!

[vbraun]

Changed branch from u/jdemeyer/upgrade_of_group_cohomology_spkg to 6576915

[fchapoton]

Changed commit from 6576915 to none