Opened 10 years ago
Closed 10 years ago
#14492 closed defect (fixed)
Modular group cohomology, version 2.1.4
Reported by: | Simon King | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | packages: optional | Keywords: | group cohomology |
Cc: | david.green@…, John Palmieri | Merged in: | |
Authors: | Simon King | Reviewers: | Volker Braun, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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
andos.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
Attachments (1)
Change History (96)
comment:1 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:3 follow-ups: 5 18 Changed 10 years ago by
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.
comment:4 Changed 10 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → Fix test script |
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.
comment:5 Changed 10 years ago by
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.
comment:6 Changed 10 years ago by
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
?
comment:7 Changed 10 years ago by
Status: | needs_work → needs_review |
---|---|
Work issues: | Fix test script |
OK, I have just posted a new spkg which uses double-minus for the command line options, and just started re-installation and test.
comment:9 Changed 10 years ago by
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.
comment:10 Changed 10 years ago by
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.
comment:12 Changed 10 years ago by
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.
comment:13 Changed 10 years ago by
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.
comment:14 follow-up: 15 Changed 10 years ago by
In spkg-check, SAGE_NUMBER_THREADS
should be changed to SAGE_NUM_THREADS
.
comment:15 Changed 10 years ago by
Replying to jhpalmieri:
In spkg-check,
SAGE_NUMBER_THREADS
should 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.
comment:16 Changed 10 years ago by
OK, fixed, and setting MAKE="make -j2"
is now enough to start tests in parallel.
comment:18 Changed 10 years ago by
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?
comment:19 Changed 10 years ago by
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.
comment:20 Changed 10 years ago by
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?
comment:21 follow-up: 22 Changed 10 years ago by
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.
comment:22 follow-up: 23 Changed 10 years ago by
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
comment:23 Changed 10 years ago by
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.
comment:24 follow-up: 25 Changed 10 years ago by
Reviewers: | → Volker Braun |
---|---|
Status: | needs_review → positive_review |
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.
comment:25 Changed 10 years ago by
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.
comment:26 follow-up: 27 Changed 10 years ago by
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.
comment:27 Changed 10 years ago by
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.
comment:28 follow-up: 32 Changed 10 years ago by
The spkg_install
seems to handle errors. If you had the log then we could investigate further ;-)
comment:29 Changed 10 years ago by
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".
comment:30 Changed 10 years ago by
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
comment:31 follow-up: 37 Changed 10 years ago by
There is no error check for
python setup.py install
Perhaps this is why success is reported if Cython fails?
comment:32 follow-up: 34 Changed 10 years ago by
Replying to vbraun:
If you had the log then we could investigate further ;-)
Still no full log has been reported...
comment:33 follow-up: 35 Changed 10 years ago by
Also: please try with sage-5.9.rc0 or later, since that includes a new version of Cython (#14452).
comment:34 follow-up: 36 Changed 10 years ago by
comment:35 Changed 10 years ago by
comment:36 Changed 10 years ago by
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.
comment:37 follow-up: 38 Changed 10 years ago by
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 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
comment:38 follow-up: 40 Changed 10 years ago by
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.
comment:39 Changed 10 years ago by
Something else: the line
DISTUTILS_DEBUG='debug'
has no effect since the variable DISTUTILS_DEBUG
is not exported.
comment:40 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
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 puttingecho Hello
or something afterpython setup.py install
to see the difference.
Great, that's it!
So, I put it to "needs work" until I fixed it.
comment:42 Changed 10 years ago by
"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.
comment:43 Changed 10 years ago by
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?
comment:44 Changed 10 years ago by
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.
comment:45 follow-up: 49 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
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'
comment:46 follow-up: 50 Changed 10 years ago by
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.
comment:47 follow-up: 54 Changed 10 years ago by
It seems to me that spkg-check
is essentially reinventing the doctest framework. Why aren't you using the standard Sage doctest framework?
comment:49 follow-up: 53 Changed 10 years ago by
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...)
comment:50 Changed 10 years ago by
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)+1by
NCPUS = int(os.environ.get('SAGE_NUM_THREADS', 1))and adjust the documentation accordingly.
But I did!!! Very strange.
comment:51 Changed 10 years ago by
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!)
comment:52 Changed 10 years ago by
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).
comment:53 Changed 10 years ago by
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 asmake
? How should it be done to temporarily disallowmake -jN
withN>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.
comment:54 follow-up: 59 Changed 10 years ago by
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.
comment:55 Changed 10 years ago by
I don't know why, but when I try to implement your suggestions, the package installation totally breaks :/
comment:56 Changed 10 years ago by
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...
comment:57 follow-up: 58 Changed 10 years ago by
ARRGH! I call $MAKE
in src/Makefile. So, forbidding to build in parallel should be done there.
comment:58 follow-up: 60 Changed 10 years ago by
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 ...
comment:59 follow-up: 61 Changed 10 years ago by
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.")
comment:60 Changed 10 years ago by
comment:61 follow-up: 64 Changed 10 years ago by
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.
comment:62 follow-up: 63 Changed 10 years ago by
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.
comment:63 follow-up: 66 Changed 10 years ago by
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.
comment:64 follow-up: 65 Changed 10 years ago by
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.
comment:65 Changed 10 years ago by
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?
comment:66 follow-up: 71 Changed 10 years ago by
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?
comment:67 Changed 10 years ago by
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
comment:68 Changed 10 years ago by
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.
comment:69 follow-up: 72 Changed 10 years ago by
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?
comment:70 Changed 10 years ago by
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
comment:71 Changed 10 years ago by
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.
comment:72 follow-up: 73 Changed 10 years ago by
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
comment:73 follow-up: 74 Changed 10 years ago by
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
...
comment:74 Changed 10 years ago by
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.
comment:75 Changed 10 years ago by
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.
comment:76 Changed 10 years ago by
Argh. Forgot to re-compile the package... So, the last timing was another example of -O1
.
comment:77 Changed 10 years ago by
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.
comment:78 Changed 10 years ago by
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
comment:79 Changed 10 years ago by
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.
comment:80 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
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.
comment:81 Changed 10 years ago by
PS: The latest changes are provided by the patch reviewer-2.1.4.patch
in the package.
comment:82 Changed 10 years ago by
Reviewers: | Volker Braun → Volker Braun, Jeroen Demeyer |
---|---|
Status: | needs_review → positive_review |
spkg looks good.
comment:83 Changed 10 years ago by
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.
comment:84 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
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.
comment:85 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
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.
comment:86 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
Sorry, one small file is still missing.
comment:87 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
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
.
comment:88 Changed 10 years ago by
Authors: | SimonKing → Simon King |
---|
comment:89 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
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.
comment:90 follow-up: 91 Changed 10 years ago by
Perhaps you meant
./makeNontips
instead of
makeNontips
comment:91 Changed 10 years ago by
Replying to jdemeyer:
Perhaps you meant
./makeNontipsinstead 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).
comment:92 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
comment:95 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
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!