Opened 5 years ago

Last modified 2 years ago

#18514 closed defect

Upgrade of group cohomology spkg — at Version 85

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-8.3
Component: packages: optional Keywords: group cohomology
Cc: jhpalmiery, vbraun, jdemeyer, schilly, frederichan, david.green@… Merged in:
Authors: Simon King Reviewers:
Report Upstream: None of the above - read trac for reasoning. Work issues: Create a new-style package with least effort
Branch: u/SimonKing/upgrade_of_group_cohomology_spkg (Commits) Commit: ad6cea059d383bbb0a97c8430caca125d2ca78b7
Dependencies: #18494 Stopgaps:

Description (last modified by jdemeyer)

OUTDATED:

The current "official" version of p_group_cohomology is 2.1.4. However, due to recent backward incompatible changes in Sage, the package would not install, respectively it would install if some header were present but wouldn't work.

Hence, an upgrade is needed. While I was at it, I have also improved the computation of Poincaré series, depth and filter degree type (the latter now uses a Hilbert-driven computation of Gröbner bases in elimination order, which works since in that setting the Hilbert function is easily available), and I added new functionality related with nilpotency.

The new spkg is at http://users.minet.uni-jena.de/cohomology/p_group_cohomology-2.1.5.spkg

Change History (85)

comment:1 Changed 5 years ago by SimonKing

  • Status changed from new to needs_review

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

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

comment:2 follow-up: Changed 5 years ago by jdemeyer

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

comment:3 in reply to: ↑ 2 Changed 5 years ago by SimonKing

Replying to jdemeyer:

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

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

comment:4 follow-up: Changed 5 years ago by jdemeyer

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

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by SimonKing

Replying to jdemeyer:

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

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

comment:6 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to SimonKing:

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

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

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

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

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:7 follow-up: Changed 5 years ago by jdemeyer

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

  1. The API of your module, that is all cdef classes and c(p)def functions that you want to export for use in other modules (in the same package or by other packages).
  1. If your module provides bindings to some library (the mtx module in your package for example), then add the declarations of the library functions in the .pxd file.
  1. Any dependencies of the above. Sage example: if you're defining a new kind of element of a ring, you will probably inherit from RingElement. In your .pxd file, you want to write cdef class MySpecialElement(RingElement). This makes RingElement a dependency, so you need to write from ... cimport RingElement in the .pxd file.

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

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by SimonKing

Replying to jdemeyer:

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

  1. The API of your module, that is all cdef classes and c(p)def functions that you want to export for use in other modules (in the same package or by other packages).
  1. If your module provides bindings to some library (the mtx module in your package for example), then add the declarations of the library functions in the .pxd file.
  1. Any dependencies of the above. Sage example: if you're defining a new kind of element of a ring, you will probably inherit from RingElement. In your .pxd file, you want to write cdef class MySpecialElement(RingElement). This makes RingElement a dependency, so you need to write from ... cimport RingElement in the .pxd file.

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

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

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

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

comment:9 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

Replying to SimonKing:

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

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

comment:10 Changed 5 years ago by kcrisman

  • Component changed from PLEASE CHANGE to packages: optional

comment:11 Changed 5 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Cope with yet another backward incompatible change of Sage

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

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

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

comment:12 Changed 5 years ago by SimonKing

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

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

comment:13 follow-up: Changed 5 years ago by SimonKing

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

comment:14 Changed 5 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Cope with yet another backward incompatible change of Sage deleted

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

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

comment:15 Changed 5 years ago by SimonKing

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

comment:16 Changed 5 years ago by frederichan

  • Cc frederichan added

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

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

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

comment:17 Changed 5 years ago by jdemeyer

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

comment:18 follow-up: Changed 5 years ago by SimonKing

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

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

comment:19 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

Replying to SimonKing:

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

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

comment:20 Changed 5 years ago by jdemeyer

In setup.py, replace

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

by

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

...

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

and use #18494.

comment:21 Changed 5 years ago by SimonKing

  • Dependencies set to #18494

I hope it is safe to make #18494 a dependency. If I understand correctly, there is only a very trivial doctest failure, and so hopefully it will be in Sage soonish.

Meanwhile I almost gave up to make my package backwards compatible with sage-3.x...

comment:22 Changed 5 years ago by SimonKing

With sage_include_directories() I will not need to construct CSAGE_PATH, right?

comment:23 Changed 5 years ago by SimonKing

I have updated the spkg, and successfully installed it on the Sage version that is provided by #18494.

comment:24 in reply to: ↑ 13 Changed 5 years ago by jdemeyer

Replying to SimonKing:

Which brings up the question again why there are all these incompatible changes in Sage.

Essentially, because of #17854 and its sub-tickets. Removing the low-level c_lib (a.k.a. libcsage) did indeed have consequences for all packages compiling against Sage.

They didn't use to occur so frequently in earlier versions of Sage, IIRC.

And they probably will not occur anymore in the future, now that #17854 is dealt with. And if there are changes, now we can just change the function sage_include_directories() from #18494.

comment:25 follow-up: Changed 5 years ago by tmonteil

Is there a reason not to make a new-style packages for p_group_cohomology ?

comment:26 in reply to: ↑ 25 Changed 5 years ago by SimonKing

Replying to tmonteil:

Is there a reason not to make a new-style packages for p_group_cohomology ?

Yes. I need someone who explains to me in ALL detail how to create a new-style package.

It begins with: How do I create an "upstream" repository for my sources? Note that I will not change the meataxe matrix wrapper that is included in the package into a wrapper for the latest meataxe upstream, as this would require to rewrite most of the c- and much of the cython-code. So, the files in the current package are an old version of meataxe with massive patches (i.e., basically a fork), plus c- and cython- and singular- and gap-code, for which this package currently is the only source.

comment:27 Changed 5 years ago by tscrim

You create a folder in the $SAGE_ROOT/build/pkgs with files similar to the others. You will also need to put the tarball with the source code in $SAGE_ROOT/upstream (and post it here or online so that it can be put on the online repo once merged) and then run sage -sh sage-fix-pkg-checksums. All of this folder should be under version control.

See also: http://doc.sagemath.org/html/en/developer/packaging.html

comment:28 Changed 5 years ago by SimonKing

It seems that there is no way around creating a new-style spkg, although I still find it odd to artificially create an upstream source in a situation where the spkg IS upstream. Sigh.

I have to

  • get a github account (or reactivate an existing account?),
  • learn how to convert the existing hg repository in the spkg to git,
  • look up how to push my private repository to github,
  • do what is said in comment:27.

Did I forget something?

Now comes the complicated part.

The spkg has three sources, namely

  1. C-MeatAxe
  2. C programs written by David Green
  3. Cython, Python, Gap and Singular code written by me.

Ad 1.

Actually the starting point is a very old version of C-MeatAxe. If I see that correctly, upstream currently isn't very active. However, compared with the version used in the spkg, upstream has changed most function names.

I apply big patches to C-MeatAxe, as I implemented Strassen multiplication. It improves performance a lot (in the old sources). However, Strassen still is not implemented in the current upstream sources. Two years ago I attended a presentation of one C-MeatAxe developer, and he said that they prefer to optimise the existing implementation rather than adding asymptotically fast implementations. It didn't convince him when I told him that in the old MeatAxe, Strassen beats school book multiplication for matrices as small as 200x200.

Ad 2.

David Green's programs have never been published independently. Hence, the spkg *is* upstream. The code uses the old C-MeatAxe extensively.

Ad 3.

My code has never been published independently. Hence, the spkg *is* upstream. It contains a Cython wrapper for the old C-MeatAxe.

Conclusion

I guess in an ideal world, I would create a new spkg for C-MeatAxe, containing my patches and my Cython wrapper and the MeatAxe executable, but starting with current upstream. I would then create a repository for Green's and my code. p_group_cohomology would then depend on the new MeatAxe spkg.

If I recall correctly, I actually have a wrapper for a recent version of MeatAxe, perhaps even with my Strassen implementation. Hence, creating the new MeatAxe spkg might be a feasible first step.

However, rewriting Green's programs in terms of the new MeatAxe could be a pain in the neck.

So, as a short term solution, I'd like to keep the proposed version 2.1.5 of my spkg, that still uses the old C-MeatAxe, with the prospect that version 3.0 should be based on the current MeatAxe upstream.

comment:29 follow-up: Changed 5 years ago by SimonKing

Sigh. Talking about complications: The spkg is too large for attaching it on trac. But sage.math.washington isn't available.

comment:30 follow-up: Changed 5 years ago by tscrim

I think you could just move the code into a tar file and the installation part of the spkg goes in the folder. The tarball just needs to be (temporarily) hosted somewhere until it gets put into the Sage upstream mirror I believe. I can host it on my github page or find a place to put it to lessen your burden. I have to go teach now, but I will take a detailed look tonight (including coming up with a better hosting plan).

comment:31 Changed 5 years ago by SimonKing

  • Description modified (diff)

comment:32 in reply to: ↑ 29 Changed 5 years ago by dimpase

Replying to SimonKing:

Sigh. Talking about complications: The spkg is too large for attaching it on trac. But sage.math.washington isn't available.

how about you make a copy of your spkg on geom.math.washington.edu or combinat.math.washington.edu somewhere readable, e.g. to /tmp ?

At least then we can pick it up and put up somewhere...

comment:33 in reply to: ↑ 30 Changed 5 years ago by SimonKing

Replying to tscrim:

I think you could just move the code into a tar file and the installation part of the spkg goes in the folder. The tarball just needs to be (temporarily) hosted somewhere until it gets put into the Sage upstream mirror I believe. I can host it on my github page or find a place to put it to lessen your burden. I have to go teach now, but I will take a detailed look tonight (including coming up with a better hosting plan).

Thank you! For now, I have put the old-style spkg at http://users.minet.uni-jena.de/cohomology/p_group_cohomology-2.1.5.spkg

comment:34 Changed 5 years ago by SimonKing

PS: The spkg uses a data base of pre-computed cohomology rings that used to be at sage.math.washington. Hence, now it is not accessible.

If the spkg can't find a cohomology ring in the data base, then it will compute it from scratch. So, most self-tests should work. However, some tests explicitly try to find data in the sage.math.washington data base. These will fail.

It seems that the data base (20-30GB) can be relocated to Jena as well. But it will take at least till next week, and when it's done I need to change the spkg to using a new default url of the data base.

comment:35 Changed 5 years ago by SimonKing

From the README file of the latest upstream sources (which is version 2.4.24 from October 2011(!)).

================================================================================
INSTALLATION (for Unix systems)
================================================================================

1) Copy Makefile.conf.dist to Makefile.conf. Edit Makefile.conf
   and fill in the necessary parameters.

Is that how it is supposed to work with a modern build system?

comment:36 Changed 5 years ago by SimonKing

There is #12103 for using MeatAxe implementation of matrix multiplication over small finite non-prime fields. But that was based on version 2.2.4, not 2.4.xx.

comment:37 follow-up: Changed 5 years ago by jhpalmieri

Do you have to convert the hg repository to a git repository? What we really need is (1) the directory build/pkgs/p_group_cohomology with spkg-install etc., and (2) a tarball containing the upstream source. Since you are the maintainer, it is up to you how to distribute that tarball, so I think if you want to distribute it with an hg repository, that's okay. If you just want to release a particular version with no repo at all included, that should be fine, too. (We don't always distribute repository information in the tarballs for other packages, for what that's worth.)

comment:38 Changed 5 years ago by jhpalmieri

For now, by the way, I think the easiest path is the right one: use your current versions of everything, don't bother trying to use newer versions of MeatAxe, etc.

comment:39 in reply to: ↑ 37 Changed 5 years ago by SimonKing

Replying to jhpalmieri:

Do you have to convert the hg repository to a git repository? What we really need is (1) the directory build/pkgs/p_group_cohomology with spkg-install etc., and (2) a tarball containing the upstream source.

Is it that easy? That would be nice.

I was told somewhere that moving from hg (even with queues?) to git is not particularly difficult. I was reluctant to use git for Sage development and I still miss hg sometimes. But using git for Sage and at the same time hg for a Sage package seems awkward to me

comment:40 Changed 5 years ago by tscrim

I agree with John: try to keep things as intact as possible. It should be possible to simply detach the SPKG.txt and sage-install scripts from spkg and put those (and sage-check and any patches) in build/pkgs/p_group_cohomology (with a package-version.txt, checksums.ini, and type files; see any other spkg). Then run sage -sh sage-fix-pkg-checksums. You should put a clean tarball of the necessary source in the upstream folder (this will be the upstream tarball that will go on the Sage mirrors), which should be everything else. Then, and only then, check that it installs (otherwise I found it deletes the tarball).

Just to be clear, I think it would be better to simply transition the spkg style here and now and consider doing upgrades later (with a version bump of your spkg).

Also have you considered bitbucket? They do hg repositories and you can keep your upstream there (and under hg control instead of git). Also they seem to be more generous than github for free (private) academic repositories.

comment:41 follow-up: Changed 5 years ago by jdemeyer

So, what is the plan here? Keep the old-style package on this ticket or change to a new-style package? In the latter case, the ticket should be set back to needs_work.

comment:42 in reply to: ↑ 41 Changed 5 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Create a new-style package with least effort

Replying to jdemeyer:

So, what is the plan here? Keep the old-style package on this ticket or change to a new-style package?

If I understood correctly, the suggestion of John and Travis was to create a new-style package with the least effort. I.e., take the current sources contained in the spkg and post them somewhere (aka "upstream"), and install new-stylish from there.

Or perhaps a little better and still feasible: Use the *proper* existing upstream for MeatAxe (even though the package uses a very old version), and only put the rest of the sources to a new upstream.

In the latter case, the ticket should be set back to needs_work.

OK.

comment:43 Changed 5 years ago by SimonKing

I have moved the documentation of the package. In case you are interested in testing an old-style version 2.1.5, see http://users.minet.uni-jena.de/cohomology/documentation/#installation

Next, I plan to create a least-effort new-style version 2.1.6. It will be basically identical with 2.1.5, except for the package style. And changing everything to the latest MeatAxe upstream will be considerable work taking some time, justifying a new ticket and a major new version 3.0.

comment:44 follow-up: Changed 5 years ago by SimonKing

Argh. There is more work to do. 2.1.5 installs, but doesn't work for non-primepower groups. The error message mentions multiple values for a keyword argument. Strange, I haven't seen that before.

comment:45 Changed 5 years ago by SimonKing

Sigh. Apparently there is also a change in the pickling of Singular objects that I am responsible for and that I have now to cope with...

comment:46 in reply to: ↑ 44 ; follow-up: Changed 5 years ago by jdemeyer

Replying to SimonKing:

Argh. There is more work to do. 2.1.5 installs, but doesn't work for non-primepower groups. The error message mentions multiple values for a keyword argument. Strange, I haven't seen that before.

I guess it's because of the Cython upgrade. Earlier versions of Cython did not give an error for code like

f(foo="hello", foo="world")

but now that's an error just like in Python.

comment:47 in reply to: ↑ 46 ; follow-up: Changed 5 years ago by SimonKing

Replying to jdemeyer:

I guess it's because of the Cython upgrade.

Probably. That error (somewhere doing mutable = True, mutable = True) is fixed. But the Singular pickling error is likely to be tricky.

Until recently, pickling of an object in the Singular interface resulted in a pickle that couldn't be unpickled, and my spkg could cope with it. By a recent change that I authored, pickling an object in the Singular interface works more likely, BUT fails with an error if it is not defined in the current base ring. So, now I get an error, where previously I just had a partial pickle.

comment:48 in reply to: ↑ 47 Changed 5 years ago by jdemeyer

Replying to SimonKing:

Until recently, pickling of an object in the Singular interface resulted in a pickle that couldn't be unpickled, and my spkg could cope with it. By a recent change that I authored, pickling an object in the Singular interface works more likely, BUT fails with an error if it is not defined in the current base ring. So, now I get an error, where previously I just had a partial pickle.

I'm afraid I can't help you there...

comment:49 Changed 5 years ago by SimonKing

In the previous versions, I already had a class GapPickler to help with pickles of Gap interface objects (by default, their content is lost when pickling, although it can be reconstructed from the string representation in most cases). I guess I could do basically the same for other interfaces.

comment:50 Changed 5 years ago by SimonKing

I replaced the spkg by a version that *almost* passes the test suite. The only remaining problem is that in the stored data the old pickling of Singular elements is used, which results in a deprecation warning.

So, my plan is: Re-compute the cohomology for all groups of order 64, store them in a database, put the database into the package, and put it online.

Then, finally create a basic version of a new-style package.

comment:51 follow-up: Changed 5 years ago by SimonKing

I created the current version 2.1.5 on my laptop and tested on the computer in my office. Most tests passed. Problems:

  • It seems that since a while, web access during doctests is not possible. It used to work, and the failing tests do work in interactive sessions. No idea how that could be fixed; I guess it was caused by a change in the doctest framework.
  • There have still been deprecation warnings when unpickling old pickled cohomology rings. Whether or not we will see the deprecation warning depends on the user: If (s)he has old pickles, then the tests will fail. If (s)he removes the old pickles before re-installing the package, then the tests will work.
  • There has been one error where a different homogeneous system of parameters was found. Mathematically there is no substantial difference, but it may be needed to change the expected output.

comment:52 in reply to: ↑ 51 Changed 5 years ago by SimonKing

Replying to SimonKing:

  • It seems that since a while, web access during doctests is not possible. It used to work, and the failing tests do work in interactive sessions. No idea how that could be fixed; I guess it was caused by a change in the doctest framework.

Strangely, these tests worked on my laptop. Is it possible that "screen" (which I use on the office computer) prevents internet access during doctests?

  • There have still been deprecation warnings when unpickling old pickled cohomology rings. Whether or not we will see the deprecation warning depends on the user: If (s)he has old pickles, then the tests will fail. If (s)he removes the old pickles before re-installing the package, then the tests will work.

Question to the reviewer: Should one add a remark in the documentation, telling that a certain deprecation warning is expected if old data is present?

  • There has been one error where a different homogeneous system of parameters was found. Mathematically there is no substantial difference, but it may be needed to change the expected output.

I get the same on my laptop. Thus I changed it, and updated the package. Now I redo the tests on my office computer.

comment:53 Changed 5 years ago by tscrim

Is there anything I can do to help right now?

comment:54 Changed 5 years ago by SimonKing

I guess I have to do some more fixes. It turns out that one can not install the spkg if before one has installed the meataxe package that I propose at #12103. Probable reason: Conflicting types in the meataxe version used here and used there.

Actually the cleanest solution would be to remove the old meataxe sources from the spkg, and rewrite the code of David Green by using the new meataxe sources. Hence, the meataxe package would become a dependency of the cohomology spkg.

But my plan was to do this (a MAJOR rewrite!) on the long run, not now. I will do some tests, to see how much work will be involved. If it merely is a cut-and-paste modification (changing ALL function and type names used in meataxe) then it might be doable.

comment:55 follow-up: Changed 5 years ago by tscrim

A more hack solution would be to make the meataxe that you include in this spkg have a different name, and so that it could be used along #12103. However, as you said, the cleanest solution would be to base this off of the meataxe spkg on #12103 as a dependency.

I would imagine that the majority of the work would just be some name/type replacements as long as you weren't doing anything too exotic...at least I doubt they made major changes to their API...

comment:56 in reply to: ↑ 55 Changed 5 years ago by SimonKing

Replying to tscrim:

I would imagine that the majority of the work would just be some name/type replacements as long as you weren't doing anything too exotic...at least I doubt they made major changes to their API...

When translating my implementation of Winograd-Strassen multiplication from meataxe-2.2.4 to meataxe-2.4.24, most of the work has indeed been cut-and-paste.

However, we talk here about changing third party code (namely David Green's), and since I didn't write it I don't know if the transition will be smooth.

comment:57 Changed 5 years ago by SimonKing

At least the data structure has remained basically the same (representing matrices as a contiguous block of memory organised in rows of the matrix, each byte contains up to 8 matrix coefficients, depending on the field size, and all arithmetic operations are done by table lookup).

comment:58 follow-ups: Changed 5 years ago by SimonKing

  • Cc david.green@… added

I am now confident that it is feasible to make the group cohomology software work with the new optional meataxe package from #12103. I managed to rewrite the stand-alone programs of David Green needed in the package, and the rest "should work" (TM) by search-and-replace operations (really, ALL names in meataxe have changed).

But I have questions concerning the new code structure.

The old spkg (p_group_cohomology-2.1.5) consists of

  • Original sources of meataxe-2.2.4, together with my "fork" of meataxe,
  • C-programs written by David Green, in src/present/,
  • GAP-functions written by David Green, in src/pGroupCohomology,
  • Singular-functions written by me, in src/pGroupCohomology,
  • Cython modules written by me, in src/pGroupCohomology,
  • documentation, including a doc builder taken (and modified) from old Sage versions,
  • a data base of cohomology rings for the groups of order 64.

For the new package, I suggest:

  • meataxe-2.4.24 is used, as provided by the package from #12103.
  • The C-programs of David Green, converted to work with the new MeatAxe, will be a new optional package called modular_resolutions-1.0 (or do you prefer modres-1.0?) depending on meataxe, adding me as a second author, as the conversion to the new MeatAxe was very nontrivial, and I also added error handling.
  • The database will also be part of modular_resolutions.
  • The Cython modules will become optional extensions in the SageMath library, say, sage.groups.modular_cohomology, depending on modular_resolutions and on database_gap. In that way, the documentation would be taken care of.
  • Where shall the Singular and GAP functions go? Is there a natural place? There is SAGE_LOCAL/share/singular/ for the Singular functions. But for GAP?

Remark: database_gap is needed because of the SmallGroups library. Technically, the upcoming modular_resolutions (or modres) package would not depend on it; but it is needed in the cohomology computations, thus, perhaps it makes still sense to add database_gap as a dependency of modular_resolutions?

comment:59 in reply to: ↑ 58 ; follow-up: Changed 5 years ago by jdemeyer

Replying to SimonKing:

or do you prefer modres-1.0?

Use whatever name that upstream uses. If you're free to choose that, I prefer modular_resolutions.

comment:60 in reply to: ↑ 58 ; follow-up: Changed 5 years ago by jdemeyer

Replying to SimonKing:

  • Where shall the Singular and GAP functions go? Is there a natural place? There is SAGE_LOCAL/share/singular/ for the Singular functions. But for GAP?

About the GAP code: is it C code for GAP or GAP code? In the latter case, can you use src/ext/gap?

comment:61 in reply to: ↑ 59 ; follow-up: Changed 5 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

or do you prefer modres-1.0?

Use whatever name that upstream uses. If you're free to choose that, I prefer modular_resolutions.

Concerning "upstream": David Green is the original upstream. But as much as I know his code has never been published outside of the old spkg. And since the rebase on top of meataxe-2.4.24 was major, I believe that I am to some extent upstream, too.

The original sources that David Green gave me are in a folder called "present". But I doubt that a user having a look at the list of optional packages would guess that "present" is about modular resolutions of p-groups. From my perspective, "modular_resolutions" is a bit lengthy. I will ask David, after all he is native speaker.

comment:62 in reply to: ↑ 60 Changed 5 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

  • Where shall the Singular and GAP functions go? Is there a natural place? There is SAGE_LOCAL/share/singular/ for the Singular functions. But for GAP?

About the GAP code: is it C code for GAP or GAP code? In the latter case, can you use src/ext/gap?

Pure GAP code, in the files src/pGroupCohomology/GapMB, .../GapMaxels and .../GapSgs.

Is src/ext/gap somehow known to Gap? Or does one need to give the full path when loading code from there into Gap?

comment:63 follow-up: Changed 5 years ago by dimpase

you might consider packaging your GAP code as a GAP package...

comment:64 in reply to: ↑ 63 ; follow-up: Changed 5 years ago by SimonKing

Replying to dimpase:

you might consider packaging your GAP code as a GAP package...

Hopefully that's a joke. I don't want to learn for yet another CAS how to contribute a package, in particular for code that I didn't author.

comment:65 in reply to: ↑ 61 Changed 5 years ago by SimonKing

Replying to SimonKing:

Replying to jdemeyer:

Use whatever name that upstream uses. If you're free to choose that, I prefer modular_resolutions.

I got answer of David Green.

  • He agrees that, by porting the code on top of the new meataxe, I became a co-author.
  • He coined his package "present", because the original purpose was to deal with presentations of modular group algebras of prime power groups.
  • If I understand correctly, he wouldn't mind renaming it.

I believe that the main purpose of the code is not to compute a presentation of a modular group algebra, but to compute a minimal projective resolution of the modular group algebra. At least that's what I am using it for.

Since the old package is called p_group_cohomology and the cohomology part will be moved to the Sage library, the package's responsibility will be to compute resolutions. Perhaps (to remind the old name) p_resolution? I find modular_resolution too long, Jeroen finds modres too abbreviated (I guess).

comment:66 in reply to: ↑ 64 Changed 5 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

you might consider packaging your GAP code as a GAP package...

Hopefully that's a joke. I don't want to learn for yet another CAS how to contribute a package, in particular for code that I didn't author.

well, by package I just meant a way to package GAP files in a way understood by GAP.

Of course, you can also use GAP's Read() command to load any GAP code from anywhere in the filesystem.

sage: gap.eval('Read("'+SAGE_LOCAL+'/foo/blah.g")') # GAP
sage: libgap.Read('"'+SAGE_LOCAL+'/foo/blah.g"') # libGAP

HTH, Dima

comment:67 follow-up: Changed 5 years ago by dimpase

by the way, would you mind renaming GAP source code files, by giving them suffix, '.g' or '.gap' ?

comment:68 in reply to: ↑ 67 Changed 5 years ago by SimonKing

Replying to dimpase:

by the way, would you mind renaming GAP source code files, by giving them suffix, '.g' or '.gap' ?

No problem. And then put it into src/ext/gap, as I was told.

comment:69 follow-up: Changed 5 years ago by SimonKing

I think I need help, as I am virtually a beginner with Makefiles, compiling c-sources, and/or creating a library.

For creating .o-files, I have the following rule in my Makefile, where

  • VPATH is the path to the sources
  • CFLAGS=-I$(MTXINC) -I$(VPATH) -Wall $(OO) -g -fPIC
  • MTXINC is the SAGE_LOCAL/include
    %.o: $(VPATH)/pgroup.h $(VPATH)/pincl.h $(VPATH)/pincl_decls.h $(VPATH)/$*.c
    	@echo "Compiling $*.c"
    	@$(CC) $(CFLAGS) -c $(VPATH)/$*.c -o $@
    

The above is a modification of MeatAxe's Makefile, which says

tmp/%.o: tmp/mk.dir src/%.c src/meataxe.h tmp/config.h
	@echo "Compiling $*.c"
	@$(CC) $(CFLAGS) -c src/$*.c -o $@

While the rule in MeatAxe's Makefile works, in my Makefile it is not executed. For example, when making pinc.o, the following is printed to stdout:

gcc -I/home/king/Sage/git/sage/local/include -I/home/king/Projekte/coho/p_group_cohomology-3.0/src/present/src -Wall  -g -fPIC    -c -o pincl.o /home/king/Projekte/coho/p_group_cohomology-3.0/src/present/src/pincl.c

In particular, the @echo "compiling ..." statement is not executed.

So, what is happening here? Is there an implicit make rule that takes precedence over the explicit rule??

And further, when I try to build an executable (say, makeInclusionMatrix), the rule says

makeInclusionMatrix: libmodres.a mim.o
	$(CC) $(LFLAGS) -o makeInclusionMatrix mim.o libmodres.a

with

  • LFLAGS=-l$(MTXLIBNAME) -L$(LIBDIR),
  • MTXLIBNAME=mtx
  • LIBDIR=$(SAGE_LOCAL)/lib

Building the executable, the output indeed contains

gcc -lmtx -L/home/king/Sage/git/sage/local/lib  -o makeInclusionMatrix mim.o libmodres.a

but aborts with an error, saying

/home/king/Projekte/coho/p_group_cohomology-3.0/src/present/src/mim.c:51: undefined reference to `MatFree'

But MatFree and other "undefined references" are defined in /home/king/Sage/git/sage/local/lib/libmtx.a!

What is happening here?

comment:70 Changed 5 years ago by SimonKing

When I instead do

gcc -L/home/king/Sage/git/sage/local/lib  -o makeInclusionMatrix mim.o pgroup.o pincl.o -lmtx

then it works. But why? And why did the other command not work?

The difference is that it says pgroup.o pincl.o instead of libmodres.a. And libmodres.a was created with ar r libmodres.a pincl.o pgroup.o aufloesung.o. Shouldn't then pgroup.o and pincl.o be available in libmodres.a anyway?

comment:71 in reply to: ↑ 69 ; follow-up: Changed 5 years ago by jdemeyer

Replying to SimonKing:

I think I need help, as I am virtually a beginner with Makefiles, compiling c-sources, and/or creating a library.

If you're manually writing a Makefile, you're already doing it wrong. The only way to reliably build a library is to use libtool.

comment:72 in reply to: ↑ 71 ; follow-up: Changed 5 years ago by SimonKing

Replying to jdemeyer:

If you're manually writing a Makefile, you're already doing it wrong. The only way to reliably build a library is to use libtool.

OK, then MeatAxe is doing it wrong as well, but after all it is third party code and I won't touch it.

Can you point me to a good introduction to libtool? What I need to create is (a) a couple of executables and (b) a library against which I can link my cython code. So, the library is not all!

comment:73 in reply to: ↑ 72 ; follow-up: Changed 5 years ago by jdemeyer

Replying to SimonKing:

OK, then MeatAxe is doing it wrong as well

Absolutely. Don't use the meataxe build system as an example.

Can you point me to a good introduction to libtool?

I don't know. For an example in Sage, you can look at the planarity package.

comment:74 in reply to: ↑ 73 Changed 5 years ago by dimpase

Replying to jdemeyer:

Replying to SimonKing:

OK, then MeatAxe is doing it wrong as well

Absolutely. Don't use the meataxe build system as an example.

Can you point me to a good introduction to libtool?

I don't know. For an example in Sage, you can look at the planarity package.

as far as I can see, meataxe does not depend on any external libraries, right? Then it's really easy to autotoolize/libtoolize. An example of a Sage package I can suggest is csdp (well, I did this libtoolization for it :-)) https://github.com/dimpase/csdp it's more complicated as it needs Lapack and Blas.

I'll email you a pdf of a readable book about autotools, so that you can get an idea what's going on. (there is no readable free online up to day text for these purposes, AFAIK).

comment:75 Changed 5 years ago by SimonKing

Still I'd like to understand what went wrong in the scenario of comment:69 and comment:70. After all, before I can autotoolize it, I should probably first be able to build it.

Last edited 5 years ago by SimonKing (previous) (diff)

comment:76 Changed 5 years ago by vbraun

As to what went wrong, I think gcc will not use static libraries if you just list them on the command line. You need -lmodres.

While building binaries is somewhat the same on all platforms, the process of building a shared library is very platform dependent. Hence Jeroen's very good advice about libtool.

comment:77 Changed 5 years ago by SimonKing

Meanwhile I really see the point of using autotools. Perhaps the experts (i.e. you) have comments/suggestions on the following, but to me it seems to work.

src/Makefile.am

AM_CFLAGS = -O3

# -----> Shared library
lib_LIBRARIES               = libmodres.a
libmodres_a_SOURCES         = pincl.c pgroup.c aufloesung.c

# -----> Headers
include_HEADERS             = modular_resolution.h

# -----> Executable (built from the shared library)

bin_PROGRAMS = makeActionMatrices makeNontips groupInfo perm2Gap makeInclusionMatrix
makeActionMatrices_SOURCES  = mam.c
makeActionMatrices_LDADD    = $(lib_LIBRARIES)
makeNontips_SOURCES         = mnt.c
makeNontips_LDADD           = $(lib_LIBRARIES)
groupInfo_SOURCES           = gi.c
groupInfo_LDADD             = $(lib_LIBRARIES)
perm2Gap_SOURCES            = perm2Gap.c
perm2Gap_LDADD              = $(lib_LIBRARIES)
makeInclusionMatrix_SOURCES = mim.c
makeInclusionMatrix_LDADD   = $(lib_LIBRARIES)

configure.ac

#                                               -*- Autoconf -*-
# Process this file with autoconf to produce a configure script.

AC_PREREQ([2.69])
AC_INIT([modular_resolution], [1.0], [simon.king@uni-jena.de])
AM_INIT_AUTOMAKE
LT_INIT
AC_CONFIG_SRCDIR([src/fp_decls.h])
AC_CONFIG_HEADERS([config.h])

# Checks for programs.
AC_PROG_CC
AC_PROG_INSTALL

# Checks for libraries.
AC_SEARCH_LIBS([MtxError], [mtx])

# Checks for header files.
AC_CHECK_HEADERS([stdlib.h string.h unistd.h meataxe.h])

# Checks for typedefs, structures, and compiler characteristics.
AC_CHECK_HEADER_STDBOOL
AC_C_INLINE
AC_TYPE_SIZE_T

# Checks for library functions.
AC_FUNC_MALLOC

AC_CONFIG_FILES([Makefile src/Makefile])
AC_OUTPUT

So far I only see a single change needed: My email address will very soon change.

comment:78 Changed 5 years ago by SimonKing

Can you help me to solve the following problem, please?

I would like to add a little test script that relies on a file test.reg that can be found in the same directory as modular_resolution.h.

In configure.ac, I have AC_CONFIG_SRCDIR([src/modular_resolution.h])

When I go to a new directory and do

/path/to/sources/of/modular_resolution/configure --prefix=`pwd`

then make check works. The file test.reg is sought in /path/to/source/of/modular_resolution/src, where it can indeed be found.

However, when I do make distcheck (which is a target provided by autotools), then first the distdir is created, and in distdir/_build the package is built and tested. Building it works fine. However, the test fails, because this time test.reg is sought in ../../src. Apparently the test is executed in distdir/_build, not in distdir/_build/src. Thus, it should be sought in ../src.

In other words, when doing "make check", an absolute path is used, whereas in "make distcheck" a wrong relative path is used.

How can that be fixed?

comment:79 follow-up: Changed 5 years ago by jdemeyer

Use $(srcdir) to refer to the source directory in Makefile.am

Another thing: avoid AM_CFLAGS = -O3. Just pass whatever flags you want for Sage to configure.

comment:80 in reply to: ↑ 79 Changed 5 years ago by SimonKing

Replying to jdemeyer:

Use $(srcdir) to refer to the source directory in Makefile.am

I did use it. Meanwhile I could fix the problem: the test script was in fact passing a wrong command line argument to some executable.

comment:81 follow-up: Changed 5 years ago by SimonKing

How to indicate that the package-to-be depends both on meataxe and I guess as well on some toolchain? I see the following in some dependencies files:

$(INST)/$(SAGE_MP_LIBRARY) $(INST)/$(ZLIB)

----------
All lines of this file are ignored except the first.
It is copied by SAGE_ROOT/build/make/install into SAGE_ROOT/build/make/Makefile.

or

$(INST)/$(MARKUPSAFE) $(INST)/$(SETUPTOOLS) $(INST)/$(DOCUTILS)

----------
All lines of this file are ignored except the first.
It is copied by SAGE_ROOT/build/make/install into SAGE_ROOT/build/make/Makefile.

The developer manual doesn't seem to mention it.

comment:82 in reply to: ↑ 81 Changed 5 years ago by SimonKing

Meanwhile I have a version of the C-part of the old group cohomology package that installs fine and even passes a self-test (namely tests that several of the installed executables work together to build the basic data for the cohomology computation of the dihedral group of order 8).

However, so far I have no idea how to make it so that the package is only built if meataxe (from #12103) is installed.

And then of course I have to put all Singular and Gap files with the appropriate file name extensions into the appropriate folders, and put the Cython and Python code into the Sage library, as an optional extension.

If I understand correctly, it is needed to add "optional: modular_resolution" basically to each single line of test (several hundreds, I guess). Is there really no way to state that a whole file should only be tested optionally? I believe it is a fairly natural thing to test an optional extension optionally.

comment:83 Changed 5 years ago by SimonKing

  • Branch set to u/SimonKing/upgrade_of_group_cohomology_spkg

comment:84 Changed 5 years ago by SimonKing

  • Commit set to ad6cea059d383bbb0a97c8430caca125d2ca78b7

I have extracted the second stand-alone ingredient of the group cohomology package.

The next step will be to relocate all the Cython and Python source files from the old package into the SageMath library, say, sage.groups.modular_cohomology.

And the final step will be to fix the doctests somehow. I think there should be a way to declare that the tests of all files in sage.group.modular_cohomology are optional.

Question to the reviewer: I made the new package depend on database_gap. In fact, what really depends on database_gap is only the to-be-created wrapper in the Sage library. Since database_gap is non-GPL, I think it should only be installed (as a dependency) when the user agrees. Is that possible with the dependency framework of packages? Or better move the dependency to the wrapper?


Last 10 new commits:

c7d75fbImplement and use Strassen-Winograd matrix multiplication in MeatAxe
9e2a8c6A very basic MeatAxe Cython wrapper
4bdd285A full wrapper for MeatAxe matrices
d889e0bImprove echelon computation in MeatAxe, and fix some compiler warnings
2e64257Doctests and error handling for MeatAxe
7c38969Fix computation of row-reduced echelon form
55a278dFix doctests when meataxe is installed
f733377Use and propagate specific return values on error in matrix-related MeatAxe functions.
efa1d75Merge branch 't/12103/meataxe' into t/18514/upgrade_of_group_cohomology_spkg
ad6cea0Add package modular_resolution

comment:85 Changed 5 years ago by jdemeyer

  • Description modified (diff)

Please update the ticket description (the last comment you made is a good starting point).

Note: See TracTickets for help on using tickets.