Modular group cohomology, version 2.1.4

[Simon King]

Because of several backward incompatible internal changes in Sage (e.g., removing SAGE_DATA and moving it to SAGE_SHARE), the current version of the optional group cohomology package p_group_cohomology did not work.

Moreover, tmp_dir used to return a string, but now it also creates a directory; for this reason, many tests failed.

And also some random generators (in GAP?) seem to have changed. For this reason, some ring presentations have changed (i.e., the rings are isomorphic, but a different presentation is obtained with an old or with a new Sage version). Hence, doctests needed to change.

In any case: The new group cohomology spkg copes with these changes. However, it will therefore not work with old Sage versions.

Further implementation changes

• Since this spkg is the upstream source, the source code is still contained in the mercurial repository. Change: Now, mercurial queues are used (this made it a bit easier for me to manage the changes while developing).
• urllib2 is used, which to me seems to work a bit better than urllib.
• The new version uses os.path.join and os.path.split, rather than string concatenation using a non-portable path separator '/'.
• The old version had a bug related with pickling: In order to keep the data base created by a user separate from the data base that is shared by all users of an installation, soft links were used. But when one saves new data into a file that used to be the name of a soft link, one should unlink the link first. This is fixed in the new version.
• The protocol output has changed. In a typical cohomology computation, several cohomology rings are involved (the ring to-be-computed, the cohomology ring of a Sylow subgroup, or of a maximal elementary abelian subgroup, etc). The new protocol output tells in which ring the current computation happens.

Change of algorithms

The first change: If no new relation has been found in degree n-1, then a complete Gröbner basis of the relation ideal is computed, even if a completeness criterion can not be applied yet. This simple change reduces the computation time for the mod-3 cohomology of J3 by several days.

The second change: By mistake, the old version would attempt to prove completeness with the Hilbert-Poincaré criterion, even if the Symonds criterion has already shown that the ring is incomplete. Now, the Hilbert-Poincaré criterion is only used if the Symonds criterion has not been conclusive.

The third change: In the old version, it was first tested that the current ring approximation contains parameters for the complete cohomology ring, and then such parameters have been constructed using the relations in the ring approximation. These relations can be rather complicated. Therefore, we now compute parameters via restriction to maximal elementary abelian subgroups. It is known that a set of elements of the ring approximation yields parameters for the cohomology ring, if and only if the cohomology of the maximal elementary abelian subgroups is finite over the restriction of these elements.

Hence, we can detect parameters even if the ring approximation is far from being complete, and we can compute in rings that are much easier to deal with. By consequence, it is now in most cases possible to prove completeness by the Symonds criterion, which relies on the explicit construction of parameters in small degrees. Our other criteria, which are able to use an existence proof for parameters of small degrees (without the need of their explicit construction) are of course still part of the package, but it became difficult to show examples in which they are actually used.

New features

There is no substantially new functionality. However, depending on how fast the reviewing goes, I could imagine to soon add a method so that gap(H) returns an algebra in the GAP interface, isomorphic to a cohomology ring H.

Documentation

The documentation can be built locally. It should look as ​here

SPKG

​p_group_cohomology-2.1.4.spkg

[Simon King]

I did not test the package on OS X yet, but only on my openSuse laptop.

The new package version should be able to compute the mod-3 cohomology of the third Janko group without manual intervention, but the verification will take a couple of more days.

In any case, it needs review!

[Volker Braun]

I get this error with the doctests:

$ sage -bt local/lib/python/site-packages/pGroupCohomology/
...
File "local/lib/python/site-packages/pGroupCohomology/factory.py", line 897, in pGroupCohomology.factory.CohomologyRingFactory._get_p_group_from_cache_or_db
Failed example:
    CohomologyRing._get_p_group_from_cache_or_db('8gp3',(8,3), from_scratch=True)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: You requested a computation from scratch. Please remove .../8gp3
Got:
    <BLANKLINE>

Maybe something wrong with the temp directory? The actual functionality seems to work, though.

[Simon King]

Hm. This should work. But there is something much more urgent concerning "should work":

I tested the package on my laptop by running the test script explicitly, which worked. But When I run it as part of the installation procedure (i.e., by export SAGE_CHECK=yes before sage -i ...), I get a total failure! There was not even a single test that passed.

Reason:

sage-runtests: error: no such option: -o
list index out of range
Usage: sage -t [options] filenames

I don't know where the "-o" comes from. To be investigated.

[Simon King]

Replying to vbraun:

I get this error with the doctests:

$ sage -bt local/lib/python/site-packages/pGroupCohomology/
...
File "local/lib/python/site-packages/pGroupCohomology/factory.py", line 897, in pGroupCohomology.factory.CohomologyRingFactory._get_p_group_from_cache_or_db
Failed example:
    CohomologyRing._get_p_group_from_cache_or_db('8gp3',(8,3), from_scratch=True)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: You requested a computation from scratch. Please remove .../8gp3
Got:
    <BLANKLINE>

Maybe something wrong with the temp directory? The actual functionality seems to work, though.

Aha. So, here I check against an error that is supposed to occur in certain settings. Hm.

I don't understand why the result is just a blank line, not the error that is actually due in this case. The line preceding this test removes the existing cohomology ring from memory but not from disk; and a computation from scratch should then fail, because it does not like to find the data in a location in which new data are supposed to be written.

[Simon King]

I think here is the problem:

    Res = os.popen('sage -t -optional -long '+f.name).read()

Could it be that in the most recent Sage version one should instead write sage -t --optional --long?

[Simon King]

OK, I have just posted a new spkg which uses double-minus for the command line options, and just started re-installation and test.

[Simon King]

No. Still all tests fail. This is with sage-5.9.rc0.

[Simon King]

The failures that I get for every single test looks like this:

pGroupCohomology.resolution.makeSpecialGroupData:
Running doctests with ID 2013-04-26-16-31-00-2eee1fc7.
Doctesting 1 file.
sage -t /home/simon/.sage/temp/linux-sqwp.site/11900/dir_vpH6Jb/file_5.py
    [0 tests, 0.00 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.0 seconds
    cpu time: 0.0 seconds
    cumulative wall time: 0.0 seconds

So, does this look like an error in the doctest framework? I can reproduce it when I directly run the spkg-check script under sage-5.9.rc0.

I tried to run the tests verbously, but it didn't help, the tests won't even start, but no reason is given.

[Simon King]

The tests of each function are written into a temporary file. One of the files looks like this:

# -*- coding: utf-8 -*-
r"""
        Return a hash value of self

        TESTS:

        The example produces files. For safety reasons, we choose files
        in a temporary directory; it will be removed as soon as Sage is quit::

            sage: tmp_root = tmp_dir()
            sage: from pGroupCohomology import CohomologyRing
            sage: CohomologyRing.set_user_db(tmp_root)
            sage: H = CohomologyRing(8,3)
            sage: H.make()
            sage: C = H.2
            sage: hash(C)==hash(copy(C))   # indirect doctest
            True
            sage: hash(C)==hash(loads(dumps(C)))
            True
            sage: D = {C:1, C^2:2}
            sage: D[copy(C)]
            1
            sage: D[copy(C)*C]
            2

"""

without a line break at the end. I am testing now if the missing line break is to blame.

[Simon King]

No, adding the line break did change nothing.

[Simon King]

One could say that I was misusing the "# optional" tag: The tests were supposed to be part of the test suite, but the tag was only in order to tell the user (not to tell the test bot): "If you don't have internet access then this test is supposed to fail; don't worry".

In the new version, that I just put on-line, the tag is replaced by "# needs internet access", which is informative to the user in case of an error, but will not interfere with the test framework.

[Simon King]

And I forgot to mention one thing: In contrast to previous versions of the spkg, it is now not required to have internet access for running the test suite. The only exceptions are the tests of two methods whose actual purpose is internet access.

[John Palmieri]

In spkg-check, SAGE_NUMBER_THREADS should be changed to SAGE_NUM_THREADS.

[Simon King]

Replying to jhpalmieri:

In spkg-check, SAGE_NUMBER_THREADS should be changed to SAGE_NUM_THREADS.

Thank you for spotting this! I have already been wondering why setting MAKE="make -j4" had no effect on the number of parallel threads in the tests.

[Simon King]

OK, fixed, and setting MAKE="make -j2" is now enough to start tests in parallel.

[John Palmieri]

On my OS X 10.8 machine, all tests passed!

[Simon King]

Replying to vbraun:

I get this error with the doctests:

$ sage -bt local/lib/python/site-packages/pGroupCohomology/
...
File "local/lib/python/site-packages/pGroupCohomology/factory.py", line 897, in pGroupCohomology.factory.CohomologyRingFactory._get_p_group_from_cache_or_db
Failed example:
    CohomologyRing._get_p_group_from_cache_or_db('8gp3',(8,3), from_scratch=True)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: You requested a computation from scratch. Please remove .../8gp3
Got:
    <BLANKLINE>

Is it reproducible?

[Simon King]

Replying to jhpalmieri:

On my OS X 10.8 machine, all tests passed!

And on my openSuse laptop, all tests passed both with sage-5.8 and sage-5.9.rc0.

[Simon King]

A question: At some point I had a syntax error in the code. Of course, the code did not compile---but sage -i ... still reported a successful installation. So, I could imagine that my spkg-install script is not correctly propagating the error.

Could you have a look if spkg-install is doing something wrong?

[Volker Braun]

I still get the from_scratch=True error, and its completely reproducible. Installation works fine (and the doctests that actually perform computations seem to be working, too). I'll attach a more complete log.

[Volker Braun]

Doctest output

[Simon King]

Replying to vbraun:

I still get the from_scratch=True error, and its completely reproducible. Installation works fine (and the doctests that actually perform computations seem to be working, too). I'll attach a more complete log.

Aha, now I think I understand what is happening!

The output looks as if you did not run the test suite (by export SAGE_CHECK=yes before installing the package), but you did run sage -t on the source files (or on their copies in local/lib/python/site-packages).

The test suite considers each doc string separately. Hence, it recursively finds out which modules, classes functions and methods are defined, extracts the doc of one item after the other, each time writing one doc string into a temporary file, and running sage -t on this temporary file. But it is not supposed to run sage -t on, say, pGroupcCohomology/factory.py.

Reason: The doc tests are supposed to demonstrate how the different sub-problems of cohomology computations are done. Hence, in the vast majority of cases, we want a computation from scratch. But if the first test puts a result into the cache, then the second test might be disturbed.

Example: One test creates the computational set-up for the cohomology of the SmallGroup(8,3) (that's the dihedral group) from scratch, but does not complete the computation. This test will create an item in the cache that is only computed out to degree zero. Another test might then demonstrate that some completely computed cohomology rings can be found in the data base, accessible by, say, CohomologyRing(8,3).

However, the factory CohomologyRing would first try to find the result in the cache, before accessing the data base on disk or in internet. Hence, it will return the degree-zero approximation of the ring. This explains why you got

45	    print H0
46	Expected:
47	    Cohomology ring of Dihedral group of order 8 with coefficients in GF(2)
48	    <BLANKLINE>
49	    Computation complete
50	    Minimal list of generators:
51	    [c_2_2: 2-Cocycle in H^*(D8; GF(2)),
52	     b_1_0: 1-Cocycle in H^*(D8; GF(2)),
53	     b_1_1: 1-Cocycle in H^*(D8; GF(2))]
54	    Minimal list of algebraic relations:
55	    [b_1_0*b_1_1]
56	Got:
57	    <BLANKLINE>
58	    Cohomology ring of Dihedral group of order 8 with coefficients in GF(2)
59	    <BLANKLINE>
60	    Computed up to degree 0
61	    Minimal list of generators:
62	    []
63	    Minimal list of algebraic relations:
64	    []
65	    <BLANKLINE>
66	

[Simon King]

Replying to SimonKing:

The test suite considers each doc string separately. Hence, it recursively finds out which modules, classes functions and methods are defined, extracts the doc of one item after the other, each time writing one doc string into a temporary file, and running sage -t on this temporary file.

PS: And it would complain if a function or a method has no doc string or no test in the doc string or a test that does not contain the name of the function/method.

[Volker Braun]

Fair enough. If you want to merge this into the sage library at one point then the doctests will have to run independently, but I guess that'll still take some time.

[Simon King]

Replying to vbraun:

Fair enough. If you want to merge this into the sage library at one point then the doctests will have to run independently, but I guess that'll still take some time.

It can not be part of the sage library, since it strongly depends on the Small Groups library, which can only be in an optional package, since it is not GPL.

But before you set it to positive review: Could you see if my spkg-install script is correct? As I have stated above, it happened to me (in preliminary versions) that there was a compilation error, but the installation script happily unpacked the data base and finished by claiming that installation of the package was successful.

[Volker Braun]

Do you actually need anything fancy from the SGL or just the list of the first N groups? I'm getting more annoyed about this stupid licensing issue, so I'll probably write a GPL replacement at one point ;-)

The spkg_install looks good to me.

[Simon King]

Replying to vbraun:

Do you actually need anything fancy from the SGL or just the list of the first N groups?

I guess the main reason for using it is: Get a concise identifier of a group with a fixed set of generators.

If I want to know the cohomology of a group G, then I need to consider restrictions to "interesting" subgroups: Maximal p-elementary abelian subgroups; Sylow p-subgroup; normalizer in G of the centre of a Sylow p-subgroup; intersection of two conjugates of a Sylow p-subgroup; ...

If I know the small groups library address of the interesting group, then I can easily look up in my data base whether the cohomology of this particular group has already been computed. Otherwise, I needed to compute the cohomology of multiple isomorphic copies of one group repeatedly.

Mathematically, it would of course be possible to lift the dependency on the small groups library. In fact, if the address in the small groups library can not be found, then a "descriptive" name (that describes the construction of this group) is chosen to access the data base. So, in these cases, my code is independent of the small groups libary.

Making it totally independent would require...

• to rewrite a couple of constructions; this would, in particular, be needed if the use of the small groups library should become optional.
• to totally rewrite the code that accesses my data bases; but actually I would like to use a "real" data base (sql?), so that rewriting will sooner or later be needed anyway.
• to rewrite almost all doctests. Since all functions and methods are tested, this would be a long and very annoying task.

The spkg_install looks good to me.

So, there is no reason why I saw it happily continue to install the package after a compilation error? Strange.

[Volker Braun]

The spkg_install seems to handle errors. If you had the log then we could investigate further ;-)

[Simon King]

For the following test, I added the command "bla" into cohomology.pyx, right after "import os". The attempt to install the resulting spkg was reported to be successful, even though of course there is an error:

...
gcc -pthread -shared -L/home/simon/SAGE/prerelease/sage-5.9.rc0/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/simon/SAGE/prerelease/sage-5.9.rc0/local/lib -lmtx -lcsage -lpython2.7 -o build/lib.linux-x86_64-2.7/pGroupCohomology/resolution.so
cythoning pGroupCohomology/cohomology.pyx to pGroupCohomology/cohomology.c

Error compiling Cython file:
------------------------------------------------------------
...

###########################################################
## Imports

import os
bla
  ^
------------------------------------------------------------

pGroupCohomology/cohomology.pyx:44:3: undeclared name not builtin: bla
building 'pGroupCohomology.cohomology' extension
gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/home/simon/SAGE/prerelease/sage-5.9.rc0/local/include/csage -I/home/simon/SAGE/prerelease/sage-5.9.rc0/devel/sage -Imtx2.2.4/src -IpGroupCohomology/c_sources -IpGroupCohomology -I/home/simon/SAGE/prerelease/sage-5.9.rc0/local/include/python2.7 -c pGroupCohomology/cohomology.c -o build/temp.linux-x86_64-2.7/pGroupCohomology/cohomology.o
pGroupCohomology/cohomology.c:1:2: error: #error Do not use this file, it is the result of a failed Cython compilation.
error: command 'gcc' failed with exit status 1

real    0m51.516s
user    0m18.015s
sys     0m3.186s
Successfully installed p_group_cohomology-2.1.4
Deleting temporary build directory

So, perhaps this is not a problem of my script but a problem of the script of Sage that is called by "sage -i".

[Jeroen Demeyer]

Instead of setting $MAKE to make (which you should never do), build with $MAKE -j1:

$MAKE -j1
if [ $? -ne 0 ]; then
   echo "Error building pGroupCohomology."
   exit 1
fi

$MAKE -j1 install
if [ $? -ne 0 ]; then
   echo "Error installing pGroupCohomology."
   exit 1
fi

[Jeroen Demeyer]

There is no error check for

python setup.py install

Perhaps this is why success is reported if Cython fails?

[Jeroen Demeyer]

Replying to vbraun:

If you had the log then we could investigate further ;-)

Still no full log has been reported...

[Jeroen Demeyer]

Also: please try with sage-5.9.rc0 or later, since that includes a new version of Cython (#14452).

[Simon King]

Replying to jdemeyer:

Replying to vbraun:

If you had the log then we could investigate further ;-)

Still no full log has been reported...

The part that failed has been reported.

[Simon King]

Replying to jdemeyer:

Also: please try with sage-5.9.rc0 or later, since that includes a new version of Cython (#14452).

It was with sage-5.9.rc0

[Jeroen Demeyer]

Replying to SimonKing:

The part that failed has been reported.

I agree, but never underestimate the amount of useful information thrown away when posting only part of a log file.

[Simon King]

Replying to jdemeyer:

There is no error check for

python setup.py install

Perhaps this is why success is reported if Cython fails?

I thought this could be it. However, if I put the following into an spkg, then the error is correctly reported, even though the exit code of setup.py is not checked:

spkg-install:

#!/usr/bin/env bash

if [ "$SAGE_LOCAL" = "" ]; then
   echo "SAGE_LOCAL undefined ... exiting";
   echo "Maybe run 'sage -sh'?"
   exit 1
fi

# test whether we are on an intel mac
if [ `uname` = "Darwin" -a "$SAGE64" = "yes" ]; then
   echo "64 bit MacIntel"
   DARWIN64=-m64; export DARWIN64;
else
   DARWIN64=""; export DARWIN64;
fi

cd src

DISTUTILS_DEBUG='debug'
python setup.py install

src/setup.py

from distutils.core import setup
from distutils.extension import Extension
from Cython.Distutils import build_ext

import shutil
import os
import subprocess

setup(
  name = "bad",
  version = "0.0",
  author = "Simon A. King",
  author_email = "simon.king@uni-jena.de",
  maintainer = "Simon A. King", 
  maintainer_email = "simon.king@uni-jena.de",
  description = "A bad example",
  ext_modules=[ 
    Extension("bad",
              sources = ["bad.pyx"],
              ),
  ],
  cmdclass = {'build_ext': build_ext}
)

src/bad.pyx

import os
bad

If I remove the last line of src/bad.pyx, then the mock package does install, and I can then do

sage: from bad import os

[Jeroen Demeyer]

Replying to SimonKing:

even though the exit code of setup.py is not checked.

The exit code is checked implicitly in spkg/bin/sage-spkg because it's simply the last command in the script. Try putting echo Hello or something after python setup.py install to see the difference.

[Jeroen Demeyer]

Something else: the line

DISTUTILS_DEBUG='debug'

has no effect since the variable DISTUTILS_DEBUG is not exported.

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

even though the exit code of setup.py is not checked.

The exit code is checked implicitly in spkg/bin/sage-spkg because it's simply the last command in the script. Try putting echo Hello or something after python setup.py install to see the difference.

Great, that's it!

So, I put it to "needs work" until I fixed it.

[Simon King]

OK, should be fixed now.

[Simon King]

"Should be fixed" means: When I broke the code on purpose, then it correctly failed to install. I am now trying to install the unbroken spkg again.

[Simon King]

I wouldn't like to revert the status to "positive review" myself. Could someone see if the potential trouble with spkg-install has been fixed?

[Volker Braun]

The error checking looks good.

You might want to add some stuff (like doc/) to .hgignore. Also, you probably don't want to keep spkg-check.orig around.

[Jeroen Demeyer]

There is a lot of garbage in the spkg (check out hg status).

In spkg-install, there is still the messing with $MAKE and this redundant line:

DISTUTILS_DEBUG='debug'

[Jeroen Demeyer]

As John mentioned, replace

NCPUS = os.environ.get('SAGE_NUMBER_THREADS')
if not NCPUS:
    NCPUS = int(multiprocessing.cpu_count()/3)+1
else:
    try:
        NCPUS = int(NCPUS)
    except:
        NCPUS = int(multiprocessing.cpu_count()/3)+1

by

NCPUS = int(os.environ.get('SAGE_NUM_THREADS', 1))

and adjust the documentation accordingly.

[Jeroen Demeyer]

It seems to me that spkg-check is essentially reinventing the doctest framework. Why aren't you using the standard Sage doctest framework?

[Jeroen Demeyer]

In spkg-install, replace

$SAGE_ROOT/sage -gap

by

gap

[Simon King]

Replying to jdemeyer:

There is a lot of garbage in the spkg (check out hg status).

Really??? Actually I already wondered about Volker's comment about spkg-check.orig. Anyway, I just posted a version that should be garbage free.

In spkg-install, there is still the messing with $MAKE

You mean: Defining MAKE temporarily as make? How should it be done to temporarily disallow make -jN with N>1?

and this redundant line:

DISTUTILS_DEBUG='debug'

So, I can remove it? Actually I can't recall where I copied this line from (probably it came from another spkg...)

[Simon King]

Replying to jdemeyer:

As John mentioned, replace

NCPUS = os.environ.get('SAGE_NUMBER_THREADS')
if not NCPUS:
    NCPUS = int(multiprocessing.cpu_count()/3)+1
else:
    try:
        NCPUS = int(NCPUS)
    except:
        NCPUS = int(multiprocessing.cpu_count()/3)+1

by

NCPUS = int(os.environ.get('SAGE_NUM_THREADS', 1))

and adjust the documentation accordingly.

But I did!!! Very strange.

[Jeroen Demeyer]

What's the point of the empty directories

src/lib
src/bin

If they are truly required in the spkg, at least remove them from .hgignore (you probably want to ship them empty!)

[Jeroen Demeyer]

OK, I see SAGE_NUM_THREADS now in spkg-check. Still, you could replace

NCPUS = os.environ.get('SAGE_NUM_THREADS')
if not NCPUS:
    NCPUS = int(multiprocessing.cpu_count()/3)+1
else:
    try:
        NCPUS = int(NCPUS)
    except:
        NCPUS = int(multiprocessing.cpu_count()/3)+1

by the one-liner

NCPUS = int(os.environ.get('SAGE_NUM_THREADS', 1))

(or replace the whole file by a call to the doctest framework).

[Jeroen Demeyer]

Replying to SimonKing:

Really??? Actually I already wondered about Volker's comment about spkg-check.orig. Anyway, I just posted a version that should be garbage free.

Probably you posted the wrong version last time.

You mean: Defining MAKE temporarily as make? How should it be done to temporarily disallow make -jN with N>1?

Replace

# MeatAxe would fail to build parallely. Therefore, although it is probably bad style:
OLD_MAKE="$MAKE";
MAKE=make; export MAKE;

cd src

$MAKE
if [ $? -ne 0 ]; then
   echo "Error building pGroupCohomology."
   exit 1
fi

$MAKE install
if [ $? -ne 0 ]; then
   echo "Error installing pGroupCohomology."
   exit 1
fi

#rm pGroupCohomology/*.c
MAKE="$OLDMAKE"; export MAKE;

by

cd src

# MeatAxe fails to build in parallel, so we build with only 1 thread:
$MAKE -j1
if [ $? -ne 0 ]; then
   echo "Error building pGroupCohomology."
   exit 1
fi

$MAKE -j1 install
if [ $? -ne 0 ]; then
   echo "Error installing pGroupCohomology."
   exit 1
fi

and this redundant line:

DISTUTILS_DEBUG='debug'

So, I can remove it?

Yes, it doesn't do anything since the variable isn't exported.

[Simon King]

Replying to jdemeyer:

It seems to me that spkg-check is essentially reinventing the doctest framework. Why aren't you using the standard Sage doctest framework?

Originally, my main reason has been to run the tests on a per-method basis, not on a per-file basis. This concerns

• running tests in parallel,
• the summary of test failures (I would like that the test suite ends with a list of methods whose tests fail or which appear not to be tested)
• execution time for each test

Also I originally wanted to work around some bugs in the sage-coverage script: Sometimes the coverage script could not correctly determine the functions defined in a module.

It could be that in the last three years the doctest and coverage framework of Sage has improved.

[Simon King]

I don't know why, but when I try to implement your suggestions, the package installation totally breaks :/

[Simon King]

I just tested: Meataxe does not build with MAKE="make -j2, even if you then say $MAKE -j1. Apparently, $MAKE is called internally, in some of the makefiles of MeatAxe. Since I patch these makefiles, it could be that I am to blame...

[Simon King]

ARRGH! I call $MAKE in src/Makefile. So, forbidding to build in parallel should be done there.

[Jeroen Demeyer]

Replying to SimonKing:

ARRGH! I call $MAKE in src/Makefile.

In that case, there is a simple solution: change the Makefile to use

$(MAKE) -j1 ...

[Jeroen Demeyer]

Replying to SimonKing:

It could be that in the last three years the doctest and coverage framework of Sage has improved.

They certainly did. I think everything in your list now works correctly, except for the per-method testing and modulo the fact that doctests and coverage are a different script.

I least I understand your motivation now, perhaps you should document this in the spkg-check file ("for historical reasons, this reimplements part of the doctest and coverage scripts, but these could (should?) be replaced by calls to the standard doctest and coverage frameworks.")

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

ARRGH! I call $MAKE in src/Makefile.

In that case, there is a simple solution: change the Makefile to use

$(MAKE) -j1 ...

I wish it were so easy... I tried this yesterday, but it didn't work.

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

It could be that in the last three years the doctest and coverage framework of Sage has improved.

They certainly did. I think everything in your list now works correctly, except for the per-method testing and modulo the fact that doctests and coverage are a different script.

Computing group cohomology is difficult. Therefore, this package does a lot of effort to recycle previous computations. But in the tests, I typically need to use a fresh computation, for otherwise I couldn't demonstrate the effect of the method being tested.

Therefore, testing on a per module basis won't be easy. I suggest that I will try to find a solution for version 3.0 of the spkg.

I least I understand your motivation now, perhaps you should document this in the spkg-check file

OK.

[Simon King]

I found another place where MAKE is used. As it turns out, it is enough to add -j1 there and only there.

Moreover, it seems that we can now build MeatAxe and David Green's programs with -O3 optimization. In previous versions, building MeatAxe with -O1 used to be faster than -O3, and David Green's programs did not work when building MeatAxe with -O3.

But now, it seems to work, presumably due to changes in gcc. I think it would make sense to test whether -O3 brings brings better speed, too.

[Jeroen Demeyer]

Replying to SimonKing:

I think it would make sense to test whether -O3 brings brings better speed, too.

You might want to try -O2 and -Os too.

[Jeroen Demeyer]

Replying to SimonKing:

Therefore, testing on a per module basis won't be easy. I suggest that I will try to find a solution for version 3.0 of the spkg.

Suggestion (absolutely not a requirement for this ticket): simply add a command to clear the cache before every test.

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

Therefore, testing on a per module basis won't be easy. I suggest that I will try to find a solution for version 3.0 of the spkg.

Suggestion (absolutely not a requirement for this ticket): simply add a command to clear the cache before every test.

Sure, this would be one possible solution (for version 3.0). Question: Would it be possible to ask the doctest framework to put this clean-up function in front of every docstring? Or would I need to insert this function myself?

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

I think it would make sense to test whether -O3 brings brings better speed, too.

You might want to try -O2 and -Os too.

I am currently waiting for pGroupCohomology.factory.unit_test64 to finish with -O3, and will later try to compare the result with the other options.

Question: Is it OK to hard-code OO="-O3" in the Makefile? Or shall this be at the choice of the user? Is there a default optimization level used by Sage when installing an spkg?

[Simon King]

First timing, with -O3:

...
#267: Walltime  59:31.94 min
      CPU-time  28:07.53 min
      Singular   2:54.90 min
      Gap-time   0:40.17 min

[Simon King]

Remark on the timing: As much as I know, the huge difference between CPU and wall time comes from reading and writing to disk. If I am not mistaken, this does not count for CPU time, but plays a substantial role here.

[Simon King]

When installing the -O2 version, I noticed that -O3 is still used on the Cython files (but it is -O2 on the files of David Green and of MeatAxe).

Question: How can I ask for using -O2 when compiling Cython generated code?

[Simon King]

Second timing, with -O2:

...
#267: Walltime  54:26.91 min
      CPU-time  25:50.69 min
      Singular   2:57.31 min
      Gap-time   0:40.97 min

[Jeroen Demeyer]

Replying to SimonKing:

Question: Is it OK to hard-code OO="-O3" in the Makefile? Or shall this be at the choice of the user?

Ideally, it should be at the choice of the user. The easiest solution is probably to use

$(CC) $(OO) $(CFLAGS) somefile.c -o somefile.o

in the Makefile. However, I wouldn't require this for positive review.

Is there a default optimization level used by Sage when installing an spkg?

No, every package handles this independently.

[Jeroen Demeyer]

Replying to SimonKing:

Question: How can I ask for using -O2 when compiling Cython generated code?

I think you would need to edit devel/sage/module_list.py

[Simon King]

Replying to jdemeyer:

Replying to SimonKing:

Question: How can I ask for using -O2 when compiling Cython generated code?

I think you would need to edit devel/sage/module_list.py

OK, so, for my package, I should look into setup.py, then...

Timing with -O1:

#267: Walltime  55:21.78 min
      CPU-time  26:29.22 min
      Singular   2:52.77 min
      Gap-time   0:39.63 min

So, currently, -O2 seems best. But we still have -Os...

[Jeroen Demeyer]

Replying to SimonKing:

OK, so, for my package, I should look into setup.py, then...

Exactly, you should add

extra_compile_args=["-O2"]

for the modules you want.

[Simon King]

Last timing (-Os):

#267: Walltime  55:35.73 min
      CPU-time  26:30.57 min
      Singular   2:54.52 min
      Gap-time   0:40.40 min

And the winner is -O2 for the non-Cython part.

[Simon King]

Argh. Forgot to re-compile the package... So, the last timing was another example of -O1.

[Simon King]

By the way, it is good to see that the two timings with -O1 are almost the same. Here it is with -Os:

...
#267: Walltime  57:08.61 min
      CPU-time  27:15.55 min
      Singular   2:46.68 min
      Gap-time   0:40.22 min

Tomorrow, I'll see what happens when choosing different options for the Cython part of the code.

[Simon King]

OO=-O2 and extra_compile_args=['-O3']:

...
#267: Walltime  46:29.31 min
      CPU-time  22:45.55 min
      Singular   2:49.84 min
      Gap-time   0:40.10 min

[Simon King]

Here are the remaining timings.

OO=-O2, extra_compile_args=['-O2']:

...
#267: Walltime  50:08.55 min
      CPU-time  24:06.87 min
      Singular   2:51.00 min
      Gap-time   0:40.32 min

OO=-O2, extra_compile_args=['-O1']:

...
#267: Walltime  51:33.45 min
      CPU-time  24:48.10 min
      Singular   2:49.59 min
      Gap-time   0:39.57 min

OO=-O2, extra_compile_args=['-Os']:

...
#267: Walltime  54:59.40 min
      CPU-time  26:07.43 min
      Singular   2:50.38 min
      Gap-time   0:40.53 min

So, at least on my laptop, it seems like a good idea to use -O2 on MeatAxe and David Green's programs, while -O3 seems best on the Cython code.

I will now try to incorporate all your remarks and update my spkg accordingly.

[Simon King]

I just posted an updated spkg, which addresses the following:

• Fine tuning of compiler options. Caveat: I am not sure if it could be overridden by the user, but I think I should not care at this point.
• spkg-install should exit with an error if setup.py throws an error, comment:38
• Remove DISTUTILS_DEBUG, do not mess with MAKE, comment:45 et al.
• Use gap, not $SAGE_ROOT/sage -gap, comment:48
• An empty src/lib and src/bin should not be shipped, comment:51
• Improve use of SAGE_NUM_THREADS, comment:52
• A note in spkg-check commenting on the reasons for replicating sage -t, comment:59

It could be that I should improve the Makefiles, but I hope this can be left for the next version.

[Simon King]

PS: The latest changes are provided by the patch reviewer-2.1.4.patch in the package.

[Jeroen Demeyer]

spkg looks good.

[Simon King]

It has turned out that, with the increased optimization level, the package won't build with old compiler versions (e.g., not with gcc 4.4.5).

Since this spkg version does not seem to be copied to the "official" list of optional spkgs: Is there still time to add a test that makes sure to install the package only when it works with the available gcc? In later versions (but not in 2.1.4 yet), I could imagine to make the optimization level depend on the available compiler.

[Simon King]

I did preserve a copy of the spkg that has been reviewed here. But I am now posting a new version that tests whether some underlying programs appear to work. If they don't, I am printing a hint concerning gcc version, and exit with an error.

If you think that this should better be done on a new ticket, then I'll do so. But I think it would be simpler to keep it here, because p_group_cohomology-2.1.4.spkg has not been put in the official repository of optional packages, yet.

[Simon King]

It seems to work (i.e., to fail) correctly.

When installing the updated spkg version in a Sage version that was built with gcc 4.4.5, it ends with

...
Testing makeNontips
makeNontips -ORLL 2 /mnt/local/king/SAGE/prereleases/sage-5.9.rc0.system-gcc/spkg/build/p_group_cohomology-2.1.4/src/present/src/test
sh: maketab: not found
sh: maketab: not found
p002.zzz: No such file or directory
make[1]: *** [makeNontips] Gleitkomma-Ausnahme
make[1]: *** Datei »makeNontips« wird gelöscht
make[1]: Leaving directory `/mnt/local/king/SAGE/prereleases/sage-5.9.rc0.system-gcc/spkg/build/p_group_cohomology-2.1.4/src/bin'
make: *** [all] Fehler 2
Error building pGroupCohomology.
You are using gcc 4.4.5
If this is older than gcc 4.6, then please consider upgrading your system-wide gcc,
or install Sage with its gcc spkg.
If your gcc version is more recent, then please inform the author.

real    0m14.620s
user    0m12.601s
sys     0m1.112s
************************************************************************
Error installing package p_group_cohomology-2.1.4
************************************************************************
...

But it installs fine on a system with gcc 4.6.

If you think that we can include this last minute change, then please review it. Otherwise, please tell me to put back the previous spkg.

[Simon King]

Sorry, one small file is still missing.

[Simon King]

OK, the spkg is updated. I verified that it does build on two systems with gcc 4.6 resp. 4.7.2, but correctly refuses to build with gcc 4.4.5, reporting a segmentation fault in makeNontips, namely

Testing makeNontips
/mnt/local/king/SAGE/prereleases/sage-5.9.rc0.system-gcc/spkg/build/p_group_cohomology-2.1.4/src/bin/maketab 2
MAKETAB Revision 1.2 (1997/09/11)
Writing tables to p002.zzz
makeNontips -ORLL 2 /mnt/local/king/SAGE/prereleases/sage-5.9.rc0.system-gcc/spkg/build/p_group_cohomology-2.1.4/src/present/src/test
make[1]: *** [makeNontips] Speicherzugriffsfehler
make[1]: *** Datei »makeNontips« wird gelöscht
make[1]: Leaving directory `/mnt/local/king/SAGE/prereleases/sage-5.9.rc0.system-gcc/spkg/build/p_group_cohomology-2.1.4/src/bin'
make: *** [all] Fehler 2
Error building pGroupCohomology.
You are using gcc 4.4.5
If this is older than gcc 4.6, then please consider upgrading your system-wide gcc,
or install Sage with its gcc spkg.
If your gcc version is more recent, then please inform the author.

The difference to the previous test is this: The test is now creating the previously missing data file p002.zzz before calling makeNontips.

[Jeroen Demeyer]

It doesn't seem to work, due to the file makeNontips being missing:

echo "Testing makeNontips"
Testing makeNontips
/mazur/release/merger/sage-5.10.beta0/spkg/build/p_group_cohomology-2.1.4/src/bin/maketab 2
MAKETAB Revision 1.2 (1997/09/11)
Writing tables to p002.zzz
makeNontips -ORLL 2 /mazur/release/merger/sage-5.10.beta0/spkg/build/p_group_cohomology-2.1.4/src/present/src/test
make[1]: makeNontips: Command not found
make[1]: *** [makeNontips] Error 127
make[1]: Leaving directory `/mazur/release/merger/sage-5.10.beta0/spkg/build/p_group_cohomology-2.1.4/src/bin'
make: *** [all] Error 2
Error building pGroupCohomology.
You are using gcc 4.8.0
If this is older than gcc 4.6, then please consider upgrading your system-wide gcc,
or install Sage with its gcc spkg.
If your gcc version is more recent, then please inform the author.

[Jeroen Demeyer]

Perhaps you meant

./makeNontips

instead of

makeNontips

[Simon King]

Replying to jdemeyer:

Perhaps you meant

./makeNontips

instead of

makeNontips

I have updated my spkg accordingly.

Could you try it again? I guess the problem on my machine has been that there was a makeNontips in the path (either broken or working, hence, the Makefile correctly detected the problem, but based on a previous installation).

[Jeroen Demeyer]

Seems to work now.

[Harald Schilly]

i've put the spkg on the server.