Closed simon-king-jena closed 11 years ago
simon-king-jena
commented
11 years ago 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!
simon-king-jena
commented
11 years ago Description changed:
---
+++
@@ -28,6 +28,10 @@
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](http://sage.math.washington.edu/home/SimonKing/Cohomology/)
+
__SPKG__
[p_group_cohomology-2.1.4.spkg](http://sage.math.washington.edu/home/SimonKing/Cohomology/p_group_cohomology-2.1.4.spkg)
vbraun
commented
11 years ago 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-jena
commented
11 years ago 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] filenamesI don't know where the "-o" comes from. To be investigated.
simon-king-jena
commented
11 years ago Work Issues: Fix test script
simon-king-jena
commented
11 years ago 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-jena
commented
11 years ago 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-jena
commented
11 years ago Changed work issues from Fix test script to none
simon-king-jena
commented
11 years ago 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-jena
commented
11 years ago No. Still all tests fail. This is with sage-5.9.rc0.
simon-king-jena
commented
11 years ago 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 secondsSo, 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-jena
commented
11 years ago 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-jena
commented
11 years ago No, adding the line break did change nothing.
simon-king-jena
commented
11 years ago 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-jena
commented
11 years ago 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.
jhpalmieri
commented
11 years ago In spkg-check, SAGE_NUMBER_THREADS should be changed to SAGE_NUM_THREADS.
simon-king-jena
commented
11 years ago Replying to @jhpalmieri:
In spkg-check,
SAGE_NUMBER_THREADSshould be changed toSAGE_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-jena
commented
11 years ago OK, fixed, and setting MAKE="make -j2" is now enough to start tests in parallel.
jhpalmieri
commented
11 years ago On my OS X 10.8 machine, all tests passed!
simon-king-jena
commented
11 years ago 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-jena
commented
11 years ago 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-jena
commented
11 years ago 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?
vbraun
commented
11 years ago 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.
vbraun
commented
11 years ago Attachment: factory-doctests.txt
Doctest output
simon-king-jena
commented
11 years ago Replying to @vbraun:
I still get the
from_scratch=Trueerror, 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 Replying to @simon-king-jena:
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.
vbraun
commented
11 years ago Reviewer: Volker Braun
vbraun
commented
11 years ago 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-jena
commented
11 years ago 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.
vbraun
commented
11 years ago 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-jena
commented
11 years ago 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...
The
spkg_installlooks good to me.
So, there is no reason why I saw it happily continue to install the package after a compilation error? Strange.
vbraun
commented
11 years ago The spkg_install seems to handle errors. If you had the log then we could investigate further ;-)
simon-king-jena
commented
11 years ago 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 directorySo, perhaps this is not a problem of my script but a problem of the script of Sage that is called by "sage -i".
jdemeyer
commented
11 years ago 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
fiThere is no error check for
python setup.py installPerhaps this is why success is reported if Cython fails?
jdemeyer
commented
11 years ago Replying to @vbraun:
If you had the log then we could investigate further ;-)
Still no full log has been reported...
jdemeyer
commented
11 years ago Also: please try with sage-5.9.rc0 or later, since that includes a new version of Cython (#14452).
simon-king-jena
commented
11 years ago simon-king-jena
commented
11 years ago 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
jdemeyer
commented
11 years ago Replying to @simon-king-jena:
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-jena
commented
11 years ago Replying to @jdemeyer:
There is no error check for
python setup.py installPerhaps 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 installsrc/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
badIf I remove the last line of src/bad.pyx, then the mock package does install, and I can then do
sage: from bad import osReplying to @simon-king-jena:
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.
jdemeyer
commented
11 years ago Something else: the line
DISTUTILS_DEBUG='debug'has no effect since the variable DISTUTILS_DEBUG is not exported.
simon-king-jena
commented
11 years ago Replying to @jdemeyer:
Replying to @simon-king-jena:
even though the exit code of setup.py is not checked.
The exit code is checked implicitly in
spkg/bin/sage-spkgbecause it's simply the last command in the script. Try puttingecho Helloor something afterpython setup.py installto see the difference.
Great, that's it!
So, I put it to "needs work" until I fixed it.
simon-king-jena
commented
11 years ago OK, should be fixed now.
simon-king-jena
commented
11 years ago "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-jena
commented
11 years ago 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?
vbraun
commented
11 years ago 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.
jdemeyer
commented
11 years ago 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'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)+1by
NCPUS = int(os.environ.get('SAGE_NUM_THREADS', 1))and adjust the documentation accordingly.
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_cohomologydid 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
os.path.joinandos.path.split, rather than string concatenation using a non-portable path separator'/'.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
CC: david.green@uni-jena.de @jhpalmieri
Component: packages: optional
Keywords: group cohomology
Author: Simon King
Reviewer: Volker Braun, Jeroen Demeyer
Issue created by migration from https://trac.sagemath.org/ticket/14492