Opened 5 years ago

Last modified 2 years ago

#18514 closed defect

Upgrade of group cohomology spkg — at Version 198

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:
Branch: u/SimonKing/upgrade_of_group_cohomology_spkg (Commits) Commit: 5e011619bc4814aeb02f785e327fb7ecff8eda1f
Dependencies: #24359 #24468 Stopgaps:

Description (last modified by SimonKing)

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

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

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

Package structure

Version 3.0 is refactored as follows:

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

Further changes

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

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

Upstream tar ball

http://users.minet.uni-jena.de/cohomology/p_group_cohomology-3.0.tar.gz

Change History (198)

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 follow-up: 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).

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

Replying to SimonKing:

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?

No, that's not possible within the dependency framework. Also, I personally don't think it's a good idea to add this kind of interactive install procedures.

Or better move the dependency to the wrapper?

The wrapper will become part of the Sage library, right? So what do you mean with "move the dependency to the wrapper"?

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

Replying to jdemeyer:

Replying to SimonKing:

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?

No, that's not possible within the dependency framework. Also, I personally don't think it's a good idea to add this kind of interactive install procedures.

Then we might be in trouble. If I understood correctly, the GPL licence does not allow that a GPL package (i.e. modular_resolution) installs a non-GPL package without asking the user.

Perhaps I should elaborate on the dependency on database_gap. The code does not link against database gap. However, it needs to be able to deal with elementary abelian groups. Sounds trivial, and it can certainly be solved abstractly, but for now the elementary abelian group is simply looked up in the small groups library.

Also, since the p_group_cohomology package is some kind of data base for cohomology rings, it is needed to assign a unique identifier to a group-with-a-fixed-minimal-generating-set. That is easier with the small groups library, but the package also allows to assign a name to a group (e.g., Co_3 for the third Conway group), and use the name as unique identifier for some item in the cohomology ring database (the fixed minimal generating set is stored along with the ring).

Or better move the dependency to the wrapper?

The wrapper will become part of the Sage library, right? So what do you mean with "move the dependency to the wrapper"?

Similarly to the MeatAxe wrapper, I plan to use OptionalExtension(). Hence, the Cython code would only result in an extension module under the condition that both modular_resolution and database_gap are installed.

So, if the current framework does not allow that installation of modular_resolutions stops for asking the users permission to install database_gap first, then we can make it so that modular_resolution only makes sure that meataxe is installed (no problem, both are GPL compatible, and modular_resolution will build and test fine), and then the OptionalExtension() will depend on both modular_resolution *and* database_gap.

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

Replying to SimonKing:

If I understood correctly, the GPL licence does not allow that a GPL package (i.e. modular_resolution) installs a non-GPL package without asking the user.

Where did you get this idea from?

comment:89 Changed 5 years ago by SimonKing

  • Description modified (diff)

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

Replying to jdemeyer:

Replying to SimonKing:

If I understood correctly, the GPL licence does not allow that a GPL package (i.e. modular_resolution) installs a non-GPL package without asking the user.

Where did you get this idea from?

From discussions on sage-devel.

comment:91 Changed 5 years ago by SimonKing

  • Description modified (diff)

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

Replying to SimonKing:

From discussions on sage-devel.

I recall that discussion and I asked exactly the same question. I don't think that was ever answered. I think it was even mentioned that we should only ask this question as a courtesy to the user, not that it was a requirement.

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

Replying to SimonKing:

Similarly to the MeatAxe wrapper, I plan to use OptionalExtension(). Hence, the Cython code would only result in an extension module under the condition that both modular_resolution and database_gap are installed.

Surely, the module will compile without database_gap. So I think you should just add meataxe as dependency for the Cython module.

The need of database_gap will make it fail at runtime. That's no problem. It's even more user-friendly, since you can show a nice error message at runtime if database_gap is not installed.

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

Replying to jdemeyer:

The need of database_gap will make it fail at runtime. That's no problem. It's even more user-friendly, since you can show a nice error message at runtime if database_gap is not installed.

OK, sounds good, except for doctests: It means that all tests in the OptionalExtension need to be marked as optional: modular_resolution database_gap, which is rather awkward to do hundreds of times. Unless of course one can somehow declare that ALL tests of a file are optional.

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

Replying to SimonKing:

It means that all tests in the OptionalExtension need to be marked as optional: modular_resolution database_gap, which is rather awkward to do hundreds of times.

Well, it's not more awkward than just optional: modular_resolution hundreds of times :-)

Unless of course one can somehow declare that ALL tests of a file are optional.

I think this is intentionally not supported. A doctest should make sense by itself, not depend on external context like the file it appears in.

comment:96 in reply to: ↑ 95 Changed 5 years ago by SimonKing

Replying to jdemeyer:

I think this is intentionally not supported. A doctest should make sense by itself, not depend on external context like the file it appears in.

Does a cohomology computation make sense if your CAS doesn't know about cohomology??

comment:97 Changed 5 years ago by jdemeyer

  • Dependencies changed from #18494 to #12103

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

It turns out that I need to expose more stuff in modular_resolution.h. My wrapper in the old spkg imports functions from various headers but I suppose it is nicer if modular_resolution-1.0 only installs a single header, modular_resolution.h, and not in addition nDiag.h etc.

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

Replying to SimonKing:

It turns out that I need to expose more stuff in modular_resolution.h. My wrapper in the old spkg imports functions from various headers but I suppose it is nicer if modular_resolution-1.0 only installs a single header, modular_resolution.h, and not in addition nDiag.h etc.

Or should I better create a folder SAGE_LOCAL/include/modular_resolution/, where *all* headers of the package will be put?

comment:100 in reply to: ↑ 99 Changed 5 years ago by dimpase

Replying to SimonKing:

Replying to SimonKing:

Or should I better create a folder SAGE_LOCAL/include/modular_resolution/, where *all* headers of the package will be put?

sure, that's the way to.

comment:101 Changed 5 years ago by SimonKing

Now I have a technical problem: I can not scp the new tarball to my account in Jena. rcp doesn't work (I suppose Jena is blocking it), scp doesn't work (my landlord is blocking ssh).

Do you have a better solution than to instrument sagemathcloud as a tool to scp the file?

comment:102 Changed 5 years ago by SimonKing

It doesn't work via sagemathcloud either: ssh: connect to host login.minet.uni-jena.de port 22: Network is unreachable

comment:103 Changed 5 years ago by dimpase

as a last resort, share it on sagemathcloud, and we'll put it somewhere.

comment:104 Changed 5 years ago by SimonKing

I need to work on the package a bit more anyway: As it turns out, I didn't put into the library what I want to import later. So, I can post the new package tomorrow, when I am at university.

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

Odd. When I try to use the resulting library in Sage, I get the complaint

/usr/lib64/gcc/x86_64-suse-linux/4.8/../../../../x86_64-suse-linux/bin/ld: /home/king/Sage/git/sage/local/lib/libmodres.a(pgroup.o): relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
/home/king/Sage/git/sage/local/lib/libmodres.a: error adding symbols: Bad value
collect2: error: ld returned 1 exit status
error: command 'gcc' failed with exit status 1

Is it not the case that autotools automatically takes care of -fPIC?

comment:106 in reply to: ↑ 105 Changed 5 years ago by SimonKing

Replying to SimonKing:

Is it not the case that autotools automatically takes care of -fPIC?

Apparently not. OK, I simply have to add it to Makefile.am.

comment:107 follow-up: Changed 5 years ago by vbraun

Autotools by itself doesn't care about pic (since it doesn't know anything about shared libraries, really). Libtool does the right thing, though.

Also, I'm confused: libmodres.a is a static library, but the ld error message refers to shared library creation.

comment:108 Changed 5 years ago by git

  • Commit changed from ad6cea059d383bbb0a97c8430caca125d2ca78b7 to 499dfbd2a4a25d3ab2f089d36e0d592ba6a0d99e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

499dfbdAdd package modular_resolution

comment:109 Changed 5 years ago by SimonKing

I have updated the tarball, consequently updated the checksum, and added a .pxd header for the package. The package contains a test ("Out of given basic data for the dihedral group of order 8, compute some additional data, and test that these data are readable and contain a certain result").

A little sanity test that I did:

sage: cython("""#!clib modres mtx
....: from sage.libs.modular_resolution cimport *
....: def test():
....:     cdef group_t *G = newGroupRecord()
....:     freeGroupRecord(G)
....: """)
sage: test()

comment:110 in reply to: ↑ 107 Changed 5 years ago by SimonKing

Replying to vbraun:

Also, I'm confused: libmodres.a is a static library, but the ld error message refers to shared library creation.

Good point. Out of lack of experience I have no preference for either static or dynamic libraries (I heard that the former can be a little faster, but not by much). And I am really not sure if my Makefile.am results in a dynamic or a static library:

lib_LIBRARIES               = libmodres.a
libmodres_a_SOURCES         = aufloesung.c aufnahme.c fileplus.c nBuchberger.c pgroup.c pincl.c slice.c urbild.c
libmodres_a_CFLAGS          = -fPIC

I guess it is dynamic. But how to create a static library (if that's desired)?

comment:111 Changed 5 years ago by SimonKing

libmodres_a_LDFLAGS = -static?

comment:112 Changed 5 years ago by SimonKing

If I should change to creating a static library then please tell me how, and if you say that I am creating the dynamic library in the wrong way then please tell me how to do better.

comment:113 follow-up: Changed 5 years ago by vbraun

Always prefer shared libraries over static libraries

With libtool the Makefile.am should contain

lib_LTLIBRARIES = libmodres.la
libmodres_la_SOURCES = aufloesung.c aufnahme.c fileplus.c nBuchberger.c pgroup.c pincl.c slice.c urbild.c

Are the upstream meataxe filenames already in German?

comment:114 in reply to: ↑ 113 Changed 5 years ago by SimonKing

Replying to vbraun:

Always prefer shared libraries over static libraries

OK.

With libtool the Makefile.am should contain

lib_LTLIBRARIES = libmodres.la
libmodres_la_SOURCES = aufloesung.c aufnahme.c fileplus.c nBuchberger.c pgroup.c pincl.c slice.c urbild.c

So, the difference is that it is lib_LTLIBRARIES, not lib_LIBRARIES, and that the file name extension is .la, not .a.

What about -fPIC? Well, I will see if it is still needed.

Are the upstream meataxe filenames already in German?

Don't mix the two things ;-)

  • Meataxe is at #12103, the filenames are English, I only added one file window.c (thus, English as well), and I will *not* do upstream's job of autotoolisation.
  • Here is modular_resolution, which was originally written by a native English speaker in Wuppertal, and he chose the filenames. By rebasing it on top of the new meataxe and by using autotools, I became part of upstream, but I didn't change the filenames.

After all, SageMath is an international project...

By the way, I am really glad that you guys convinced me of using autotools. It really is so much easier!

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

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

There is one more file whose new location has to be determined: GroupNames.sobj. Thus, it is a Sage pickle, and it contains a dictionary assigning human-readable names (such as DihedralGroup(8)) to some addresses from the small groups library (such as SmallGroup(8,3)).

So, questions:

  • What is a natural location for a data file that is part of the SageMath distribution?
  • Do you happen to know a Gap function that can provide human-readable names (and other useful information) for a wider range of groups?

comment:116 Changed 5 years ago by git

  • Commit changed from 499dfbd2a4a25d3ab2f089d36e0d592ba6a0d99e to cd644a301bd2a9c57b5c719bb47a4733c75e347b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cd644a3Add package modular_resolution

comment:117 Changed 5 years ago by SimonKing

I have posted an updated version of the upstream tarball. After changing from lib_LIBRARIES to lib_LTLIBRARIES, using -fPIC was not needed any longer.

Now, I try to move the Cython wrappper.

comment:118 in reply to: ↑ 115 Changed 5 years ago by SimonKing

Replying to SimonKing:

  • What is a natural location for a data file that is part of the SageMath distribution?

Perhaps I should add that the old spkg used several default locations for data.

  • SAGE_SHARE/pGroupCohomology for data that are obtained during installation of the package, which are shared among different users of one Sage installation, and which are not supposed to be modified by a user, unless (s)he knows what (s)he is doing and has write permission.
  • DOT_SAGE/pGroupCohomology for data that are created by a user, and which are only available to this user.

Is SAGE_SHARE/pGroupCohomology still a good location for shared data? If that is the case, then I think GroupNames.sobj should go there. But how? If I understand correctly, SAGE_SHARE is not under version control. Is it a possibility to add these data files in the modular_resolution tarball, and modifying its Makefile so that the data are installed in SAGE_SHARE/pGroupCohomology?

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

Replying to SimonKing:

There is one more file whose new location has to be determined: GroupNames.sobj. Thus, it is a Sage pickle

Does it really have to be a pickle? Why not just a Python file?

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

Replying to jdemeyer:

Does it really have to be a pickle? Why not just a Python file?

Well, I could also put it directly into the code (the file is loaded in exactly one spot in the cohomology code). So, right, I could get rid of the pickle.

But can the shared database remain in SAGE_SHAR/pGroupCohomology? I think it is reasonable that the new modular_resolution package shall provide the cohomology ring data base, even though the actual code to access the data will only be in the Sage library.

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

Replying to SimonKing:

But can the shared database remain in SAGE_SHARE/pGroupCohomology?

Of course. Just as an example, the conway_polynomials database is also in $SAGE_SHARE.

comment:122 in reply to: ↑ 121 Changed 5 years ago by SimonKing

Replying to jdemeyer:

Of course. Just as an example, the conway_polynomials database is also in $SAGE_SHARE.

Hrmph. I need more help.

I created a data/ directory and put the untar'd cohomology data into that directory. Thus, data/ now has 268 sub-directories, namely 8gp3/ and 64gp1/ up to 64gp267/. The aim is to copy all these sub-directories into $(prefix)/share/pGroupCohomology (and to create $(prefix)/share/pGroupCohomology in the first place).

What I tried is to put the following into data/Makefile.am:

dbdir    = $(prefix)/share/pGroupCohomology
nobase_db_DATA  = $(wildcard *gp*)

expecting that "make install" would create the directory and then recursively (because of nobase_) copy all folders *gp* into it.

However, autoreconf complains that wildcard *gp*: non-POSIX variable name, and when I do make install, then there is no share/pGroupCohomology, and from the log I don't see any attempt to install any data.

What do I need to do instead?

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

Perhaps I should have a single tar file containing all the data, and then add an install-data-hook that untars the data file?

comment:124 in reply to: ↑ 123 Changed 5 years ago by SimonKing

Replying to SimonKing:

Perhaps I should have a single tar file containing all the data, and then add an install-data-hook that untars the data file?

It works *almost*: I can do make install and make clean and make uninstall and so on, but nonetheless make distcheck fails. And I don't understand why.

I have this in the top-level Makefile.am:

ACLOCAL_AMFLAGS = -I m4
SUBDIRS = src

dbdir      = $(datadir)/pGroupCohomology
dist_db_DATA    = group_cohomology_database.tar

install-data-hook:
        cd $(dbdir) && tar xf $(dist_db_DATA)

uninstall-hook:
        rm -r $(dbdir)

clean-local:
        rm $(dbdir)/$(dist_db_DATA)

When I run make distcheck, then the command cd $(dbdir) in install-data-hook fails. The odd reason: datadir is /home/king/Projekte/coho/bin/modular_resolution-1.0/_inst/share (I checked that), but during "make distcheck" the data are installed NOT in datadir but in /tmp/am-dc-11743//home/king/Projekte/coho/bin/modular_resolution-1.0/_inst/share/pGroupCohomology. Consequently, when I want to cd into dbdir, the folder can't be found.

Do you have an idea why distcheck installs data not into datadir but into a temporary folder, whereas make install correctly installs into datadir?

comment:125 Changed 5 years ago by SimonKing

Got it. make distcheck uses a staged install, using the DESTDIR variable. And I somewhere found the comment: "Support for DESTDIR is implemented by coding it directly into the install rules. If your Makefile.am uses a local install rule (e.g., install-exec-local) or an install hook, then you must write that code to respect DESTDIR."

comment:126 Changed 5 years ago by git

  • Commit changed from cd644a301bd2a9c57b5c719bb47a4733c75e347b to 9999966fb6d48fa4516e7d11220efab69ed457e2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9999966Add package modular_resolution

comment:127 Changed 5 years ago by SimonKing

Interesting commit number: 9999966...

Anyway. I have again updated the upstream tarball. Now, it contains the database of the modular cohomology rings of all groups of order 64 that is part of the old spkg. Recall that a much larger database is not part of the tarball but is available in internet.

And of course it will be needed to have code in the Sage library for actually reading the data. But I hope that's ok.

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

I can easily put the Gap code into $SAGE_ROOT/src/ext/gap/modular_cohomology/: It is under version control.

However, $SAGE_ROOT/local/share/singular is not under version control. Does that mean I should better use a different location for the Singular code?

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

Replying to SimonKing:

Singular code?

If it's Singular code, it should go into src/ext/singular by analogy with src/ext/gap and src/ext/pari.

comment:130 in reply to: ↑ 129 Changed 5 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

Singular code?

If it's Singular code, it should go into src/ext/singular ...

... which I need to create first, but of course I can do.

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

A question about copyright statements: When I put all the Cython and Python files into the Sage library, should I preserve the old copyright statements such as Copyright (C) 2009 Simon A. King <simon.king@uni-jena.de>, or should I add new ones, i.e. Copyright (C) 2015 Simon A. King <simon.king@uni-koeln.de>?

comment:132 in reply to: ↑ 131 Changed 5 years ago by dimpase

Replying to SimonKing:

A question about copyright statements: When I put all the Cython and Python files into the Sage library, should I preserve the old copyright statements such as Copyright (C) 2009 Simon A. King <simon.king@uni-jena.de>, or should I add new ones, i.e. Copyright (C) 2015 Simon A. King <simon.king@uni-koeln.de>?

well, it's good to have an up to date contact info there, while keeping the copyright dates, such as Copyright (C) 2009, 2015 Simon A. King <simon.king@uni-koeln.de>

comment:133 Changed 5 years ago by SimonKing

  • Work issues changed from Create a new-style package with least effort to Debug the modification of the modular resolution code

comment:134 Changed 5 years ago by SimonKing

Information on "Debug the modification of the modular resolution code": The old meataxe had row and column numbers one-based, the new has it zero-based. It seems to me that in some locations of the modular resolution code, this needs to be coped with.

comment:135 in reply to: ↑ description Changed 4 years ago by SimonKing

The work is almost done, but according to discussions on sage-devel I should not proceed according to the original ticket description. Hence, in this comment, I explain why, and then I will change the ticket description.

Replying to SimonKing:

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. Note that such backward incompatible changes happened repeatedly.

And it has happened again: Meanwhile I offer an old-style spkg version 2.1.6, and both new versions do nothing but coping with backward incompatible changes.

The new package shall be modularised as follows.

  • First, install meataxe (see #12103) and database_gap.

This is still the case --- based on meataxe, there is an alternative implementation of matrices in the Sage src tree, and that will be used

Note that soon I will create a ticket to add a few methods to the meataxe wrapper that I intend to use in my cohomology programs.

  • Third, get the sources for modular_resolution-1.0. It is based on code of David Green, which I refactored rather extensively:

... 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.

I was told on sage-devel to not move the code to the Sage src tree, but make it an external package that depends on the meataxe spkg, and is formed by two parts: The first is a C library together with some executables provided by what above was called modular_resolution-1.0, and a pip-installable package formed by cython and python files. Reasons:

  • My package adds functionality, but doesn't modify existing functionality.
  • In order to use it, it isn't needed to cimport from my package.
  • People wouldn't like to have more than 10,000 lines of code in the src tree that is useless without some optional package.
  • There currently is no easy way to make all doctests in one file optional.

That said, I still believe that for me working in the src tree would be better; for the record:

  • Currently I do the development in the src tree anyway. So, moving stuff from the src tree into an external package is a nontrivial additional step for me.
  • Building the docs of python/cython code outside of the src tree is a nontrivial additional step for me.
  • Being in the src tree would make it less likely that backward incompatible Sage changes breaking my package will happen unnoticed again.
  • Having the documentation of my optional package in the Sage docs would make more people aware, so that they would be more likely to use my programs in research.

comment:136 Changed 4 years ago by SimonKing

  • Branch u/SimonKing/upgrade_of_group_cohomology_spkg deleted
  • Commit 9999966fb6d48fa4516e7d11220efab69ed457e2 deleted
  • Description modified (diff)
  • Work issues Debug the modification of the modular resolution code deleted

comment:137 Changed 4 years ago by SimonKing

My old-style package does contain a setup.py for installing the cython and python code, and it is able to build a documentation (basically by a modified copy of an old version of Sage's docbuilder). However, very likely both is not how it should be done.

I have three requests to the reviewers:

  1. Please point me to a good example of a setup.py in a new-style spkg for installing Cython code that cimports Sage types (such as Element and Parent).
  2. Please point me to a good example of a new-style spkg that teaches me how to build a nice documentation based on docstrings.
  3. Please tell me what I need to do in order to have the cohomology spkg automatically rebuilt as soon as the cimported Sage types change.

comment:138 Changed 4 years ago by SimonKing

  • Dependencies changed from #12103 to #12103 #21437

I need some modifications to the MeatAxe wrapper, and I'd appreciate it to be reviewed tat #21437.

comment:139 Changed 4 years ago by SimonKing

  • Branch set to u/SimonKing/upgrade_of_group_cohomology_spkg

comment:140 Changed 4 years ago by SimonKing

  • Commit set to f1c0dbd51d0cab48b1c21f3faca9c7e2245764e4

I guess it is better to provide code at some point. There are many doctest failures (many of them just because of different logging), none of them is marked optional, but as much as I see it would be possible to do non-trivial computations with the new code version.

INSTALLATION

HOWEVER...

The current branch does everything in Sage's src tree, which (as I was told) is evil.

But perhaps some of you can answer the questions that I have posted earlier: What do I need to do with the current code base in order to create a pip-installable module and build its documentation?


Last 10 new commits:

93d59dcUse Python's logging module for logging
a6144a3Add reset-function, fix some tests
09e76a6New return format of verifyGroupIsAbelian; use some boilerplate
16c7912Replace MatEchelonize by full echelon computation; use set_unsafe_int resp. set_unsafe more carefully
36bcb8efix various doctests
2cc1070Fix MTX matrix slicing; fix doctest continuation
d900786Add docs to the reference manual. Remove Ordinals(), fix one occurence of get_slice.
d384968Use boilerplate instead of set_unsafe_int, since this is affected from meataxe being static library
8fbaad9_reset() now involves the logger. Fix some tests
f1c0dbdFix usage of some matrix related functions

comment:141 Changed 4 years ago by git

  • Commit changed from f1c0dbd51d0cab48b1c21f3faca9c7e2245764e4 to 710f6f5848226bffd0895ed30f93c46b421c8460

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

ee392f5Make logging at 'info' level less verbose
710f6f5Let unit_test64 accept keyword arguments

comment:142 follow-ups: Changed 4 years ago by SimonKing

There are good news:

I have just finished unit_test_64, which does a computation of the cohomology rings of all groups of order 64, compares the results with the data in the data base, and provides timings.

The results obtained with the new package version coincide with the old data. Moreover, the computation on a laptop with Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz and 8GB of RAM only took 27:32 minutes of wall time resp. 15:56 minutes of CPU time, which is pretty fast.

Out of curiosity, I will repeat the test on the same machine with the old package version, and see how long it takes.

comment:143 in reply to: ↑ 142 Changed 4 years ago by SimonKing

Replying to SimonKing:

Out of curiosity, I will repeat the test on the same machine with the old package version, and see how long it takes.

It takes roughly the same time. Actually the old package was two minutes faster.

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

Replying to SimonKing:

I have just finished unit_test_64, which does a computation of the cohomology rings of all groups of order 64, compares the results with the data in the data base, and provides timings.

The results obtained with the new package version coincide with the old data. Moreover, the computation on a laptop with Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz and 8GB of RAM only took 27:32 minutes of wall time resp. 15:56 minutes of CPU time, which is pretty fast.

Since the old version was a little faster, I did some profiling. The profiler told me that much time is spent (both with the old and the new version) on do_system. With the help of sage-devel, I could track it down to several calls to system(), which all turned out to be useless.

After removing all the system calls, the performance has drastically improved! On the same machine, unit_test_64 now only needed 9:26 minutes wall time resp. 8:37 min CPU time, which is only about one third resp. one half of the old timing, and I guess a new world record for the computation of the modular cohomology rings of all groups of order 64.

I currently do not have the bandwidth to post the new version of modular_resolution-1.0.tar.gz, and will do so on Monday.

comment:145 Changed 4 years ago by git

  • Commit changed from 710f6f5848226bffd0895ed30f93c46b421c8460 to 0eed64d245f15296a3beda5535cabd3c417ed25c

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

966ec0eFix documentation of unit_test_64
0aa2980Update modular_resolution-1.0, removing all system calls
3e130aeAlways try to save the basic ring setup on disc
d11e678Fix attribute reconstruction, to make unit_test_64 work with 'nosave'
e5f88a4Improve handling of global options
d9ffcf4Various small speedups
e17e4dfDocument and test _rowlist_()
291f4dcSome minor tweaks for performance
0eed64dMerge branch 't/21437/fix-mtx-matrices' into t/18514/upgrade_of_group_cohomology_spkg

comment:146 in reply to: ↑ 144 Changed 4 years ago by SimonKing

Replying to SimonKing:

Replying to SimonKing:

I have just finished unit_test_64, which does a computation of the cohomology rings of all groups of order 64, compares the results with the data in the data base, and provides timings.

The results obtained with the new package version coincide with the old data. Moreover, the computation on a laptop with Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz and 8GB of RAM only took 27:32 minutes of wall time resp. 15:56 minutes of CPU time, which is pretty fast.

Since the old version was a little faster, I did some profiling. The profiler told me that much time is spent (both with the old and the new version) on do_system. With the help of sage-devel, I could track it down to several calls to system(), which all turned out to be useless.

After removing all the system calls, the performance has drastically improved! On the same machine, unit_test_64 now only needed 9:26 minutes wall time resp. 8:37 min CPU time, which is only about one third resp. one half of the old timing, and I guess a new world record for the computation of the modular cohomology rings of all groups of order 64.

I currently do not have the bandwidth to post the new version of modular_resolution-1.0.tar.gz, and will do so on Monday.

Done.

And in addition I obtained some further minor improvements, basically by using a C profiler. The computation of the modular cohomology rings of all groups of order 64 on my laptop now takes about 6:50 min of wall time resp. 6:10 min of CPU time (plus the time spent by Singular and GAP, which counts for the wall time but not for the CPU time).

I started to do time series to optimise the default choice of some options, but the standard deviation is considerable. So, I guess I'll stick with the C profiler...

Let me come back to my question: How can I move this away from the src tree and make it an independent package while being able to build a nice documentation and to run doc tests? Note that all this did work in the old spkg, but certainly can be done a lot better.

comment:147 Changed 4 years ago by git

  • Commit changed from 0eed64d245f15296a3beda5535cabd3c417ed25c to 1c43593103884c96021568553cf37d2b3884bf3a

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

1c43593Sanitise handling of global options

comment:148 Changed 4 years ago by git

  • Commit changed from 1c43593103884c96021568553cf37d2b3884bf3a to 54e1a0071d54fe3bc6189e2faa3af5d956b67f2a

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

aa1985aFix tests in resolution.pyx
5554cefUse SageMath's framework for _richcmp_ of elements
fbbdff9Use boilerplate to avoid transformation of matrix rows into lists
77bdc99Cache whether the group is abelian
3ce31d4Remove more instances of _rowlist_
d04b020update of modular_resolution
8d2c563Change modular_resolution to a more efficient monomial ordering
e8759a7Documentation of new functions. Fix doctest in cochain.pyx
3413fadFixing almost all tests in cohomology.pyx. Change Simon King's address
54e1a00Remove 'timing' option. Remove commented out old code. Fix tests in modular_cohomology.pyx

comment:149 Changed 4 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_info

If MeatAxe was a standard spkg (which might actually be something to consider) and the SmallGroups library was GPL compatible (something which the authors refuse), I could promote the modular group cohomology computation as a new standard package: With the changes that I just pushed, all tests pass (on my machine at least), and moreover the computations have still become faster.

I have changed the status to "needs info", stating my information request in the ticket description.

comment:150 in reply to: ↑ description Changed 3 years ago by jdemeyer

Replying to SimonKing:

  • Please point me to an example of a new style spkg that builds a C library and Cython modules (not in Sage's src tree) that link against the library.

This is most easily treated as two separate things (the C library + the Cython wrapper). There are plenty examples for each of those two. Simple examples are https://github.com/graph-algorithms/edge-addition-planarity-suite for the C library and https://github.com/defeo/cypari2 for the Cython wrapper.

  • Please point me to an example of a new style spkg that builds documentation based on doc strings in the Cython code.

https://github.com/sagemath/cysignals (result: http://cysignals.readthedocs.io/en/latest/)

See also http://opendreamkit.org/2017/06/09/CythonSphinx/

These are all standard Sage packages (planarity, cypari and cysignals)

comment:151 Changed 3 years ago by SimonKing

  • Dependencies changed from #12103 #21437 to #12103 #21437 #23352

comment:152 Changed 3 years ago by git

  • Commit changed from 54e1a0071d54fe3bc6189e2faa3af5d956b67f2a to 63355a85360616cff0d4befb3d5e0c78b4d129e5

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

08b1724Use SageMath's framework for _richcmp_ of elements
b3156e4Use boilerplate to avoid transformation of matrix rows into lists
883dae6Cache whether the group is abelian
c80d8a5Remove more instances of _rowlist_
6100c34update of modular_resolution
c7a1ae9Change modular_resolution to a more efficient monomial ordering
33bcb59Documentation of new functions. Fix doctest in cochain.pyx
eb3b6e2Fixing almost all tests in cohomology.pyx. Change Simon King's address
8f7001aRemove 'timing' option. Remove commented out old code. Fix tests in modular_cohomology.pyx
63355a8Get the cohomology code to build

comment:153 Changed 3 years ago by git

  • Commit changed from 63355a85360616cff0d4befb3d5e0c78b4d129e5 to ae029297d600471d77f3de795c9ad8d726988a3b

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

d0102a4update of modular_resolution
919d7f7Change modular_resolution to a more efficient monomial ordering
15464b4Documentation of new functions. Fix doctest in cochain.pyx
72837b7Fixing almost all tests in cohomology.pyx. Change Simon King's address
67b3ccfRemove 'timing' option. Remove commented out old code. Fix tests in modular_cohomology.pyx
5360988Get the cohomology code to build
aa07e43Modified handling of MtxLibDir
32a411dUse a fixed copy of MtxLibDir
55ddf00More clearly document from_scratch option
ae02929Use meataxe_init() instead of manually setting the parameters

comment:154 follow-up: Changed 3 years ago by SimonKing

  • Dependencies changed from #12103 #21437 #23352 to #21437

Now I guess the meataxe problems are basically dealt with in #23352, #23400, #23399 and #21437. Therefore I have rebased the current cohomology code.

The current layout is:

  1. MeatAxe is a separate optional package
  2. The C code of David Green to compute minimal resolutions is a separate package, which also installs the data base of cohomology rings of all groups of order 64 and which also installs certain GAP and Singular code interpreted modules.
  3. The Python and Cython code is in the Sage source tree. The Cython code is optional extension module, which needs the package from 2. as a build time dependency. Moreover, at run time, it depends on the small groups library.

What layout should there be, instead? I guess MeatAxe has to stay separate. But shall everything else be included in one optional (new-style) spkg? So, that spkg would provide 2. and 3.?

comment:155 in reply to: ↑ 154 ; follow-up: Changed 3 years ago by SimonKing

Replying to SimonKing:

Now I guess the meataxe problems are basically dealt with in #23352, #23400, #23399 and #21437. Therefore I have rebased the current cohomology code.

The current layout is:

  1. MeatAxe is a separate optional package
  2. The C code of David Green to compute minimal resolutions is a separate package, which also installs the data base of cohomology rings of all groups of order 64 and which also installs certain GAP and Singular code interpreted modules.
  3. The Python and Cython code is in the Sage source tree. The Cython code is optional extension module, which needs the package from 2. as a build time dependency. Moreover, at run time, it depends on the small groups library.

Apparently, the Cython code can not stay in the source tree, as it doesn't qualify as optional extension module.

Possible layouts:

  1. Two packages
    • meataxe (as it is now)
    • everything else put into one upstream tarball, giving rise to one new-style spkg
  1. Three packages
    • meataxe (as it is now)
    • C code in one upstream tarball, giving rise to a new-style spkg "modular_resolution" that provides a C library and some executables but would probably be of no interest on its own?
    • Cython and Python code in another upstream tarball, giving rise to another new-style spkg "p_group_cohomology" that depends on "modular_resolution".

Would it be OK to have an spkg "modular_resolution" if it basically has no other purpose than being a dependency for "p_group_cohomology"?

And I still wonder what to do with the GAP code and the Singular code. On the one hand, such code clearly belongs to src/ext/gap respectively to src/ext/singular. On the other hand, an spkg is not supposed to inject code into src/ext.

Is it ok to put the relevant files into src/ext even when the optional packages are not installed? I would think so; after all, the files I am talking about are not particularly large.

comment:156 in reply to: ↑ 155 ; follow-up: Changed 3 years ago by tscrim

Replying to SimonKing:

Apparently, the Cython code can not stay in the source tree, as it doesn't qualify as optional extension module.

Can you elaborate a bit more on this? I don't quite understand why not from a quick glance.

Possible layouts:

  1. Two packages
    • meataxe (as it is now)
    • everything else put into one upstream tarball, giving rise to one new-style spkg
  1. Three packages
    • meataxe (as it is now)
    • C code in one upstream tarball, giving rise to a new-style spkg "modular_resolution" that provides a C library and some executables but would probably be of no interest on its own?
    • Cython and Python code in another upstream tarball, giving rise to another new-style spkg "p_group_cohomology" that depends on "modular_resolution".

Would it be OK to have an spkg "modular_resolution" if it basically has no other purpose than being a dependency for "p_group_cohomology"?

Yes, this is perfectly fine to have this kind of granularity. Although if it could not have any independent interest/development, then it probably would be better to fold it into p_group_cohomology from a more macro-scale maintenance viewpoint. I guess it makes the p_group_cohomology less homogeneous as a project? I also had the impression that it would be able to be a standalone library.

And I still wonder what to do with the GAP code and the Singular code. On the one hand, such code clearly belongs to src/ext/gap respectively to src/ext/singular. On the other hand, an spkg is not supposed to inject code into src/ext.

I feel like those are no different than having Cython files whose compilation depends on an optional spkg or Python files that are only useful if an optional spkg is installed. So, by analogy, they should not be injected by the spkg during its installation, but instead part of the main Sage git control.

Is it ok to put the relevant files into src/ext even when the optional packages are not installed? I would think so; after all, the files I am talking about are not particularly large.

IMO, I think it is fine to put it in the source tree if it is meant to be something for Sage as suggested above. I'm not sure if I have a complete understanding of everything at this point to say anything definitive about what you should do in this situation. Also, someone might come along and say you should do it all in a pip package, but then I feel it is more likely to break on future upgrades of Sage.

comment:157 in reply to: ↑ 156 ; follow-up: Changed 3 years ago by SimonKing

Replying to tscrim:

Replying to SimonKing:

Apparently, the Cython code can not stay in the source tree, as it doesn't qualify as optional extension module.

Can you elaborate a bit more on this? I don't quite understand why not from a quick glance.

I have asked about it on sage-devel, and was told that a cython OptionalExtension should only be used if an existing functionality (i.e., a functionality that is provided by vanilla Sage) can be implemented in a different (better) way if the user has installed some optional package. Example: You have matrices over GF(p^n) with p>2 and n>1 in Vanilla Sage, but if you install meataxe then a much faster implementation is available. Or you could also think of different graph backends.

However, the situation here is different: This is about a functionality that is not available in vanilla Sage and can only be provided if the user installs some optional package. I was told on sage-devel that I shouldn't use OptionalExtension in that situation.

Would it be OK to have an spkg "modular_resolution" if it basically has no other purpose than being a dependency for "p_group_cohomology"?

Yes, this is perfectly fine to have this kind of granularity. Although if it could not have any independent interest/development, then it probably would be better to fold it into p_group_cohomology from a more macro-scale maintenance viewpoint. I guess it makes the p_group_cohomology less homogeneous as a project? I also had the impression that it would be able to be a standalone library.

David Green's code to compute resolutions of modular group algebras of finite p-groups has originally been a bunch of executables operating on data stored on disk. I made a standalone library out of it.

So, it could be used on its own, but I guess nobody has ever done.

Currently, I am developer for both the C / Gap code that I inherited from David Green, and for the Cython / Python / Singular code that I wrote myself. I do not foresee active development for the C code in the near future. So, from a development point of view, it would be easier to make a separate package out of the C code --- it is not supposed to change in future versions. On top of that, the active development will take place in the Cython / Python / Singular part of the code, which thus should be another separate package.

And I still wonder what to do with the GAP code and the Singular code. On the one hand, such code clearly belongs to src/ext/gap respectively to src/ext/singular. On the other hand, an spkg is not supposed to inject code into src/ext.

I feel like those are no different than having Cython files whose compilation depends on an optional spkg or Python files that are only useful if an optional spkg is installed. So, by analogy, they should not be injected by the spkg during its installation, but instead part of the main Sage git control.

It is more like "some code snippets which can be used by some optional package that the user may install". So, I could understand that we wouldn't like to have these code snippets in the src tree. However, they are supposed to be used in Sage's GAP and Singular, and thus it makes sense to put them into src/sage/libs. That's the dilemma...

Is it ok to put the relevant files into src/ext even when the optional packages are not installed? I would think so; after all, the files I am talking about are not particularly large.

IMO, I think it is fine to put it in the source tree if it is meant to be something for Sage as suggested above. I'm not sure if I have a complete understanding of everything at this point to say anything definitive about what you should do in this situation. Also, someone might come along and say you should do it all in a pip package, but then I feel it is more likely to break on future upgrades of Sage.

I just thought: What does database_gap do? It is installing databases into Sage's version of GAP. What is the purpose of the GAP / Singular files that are used in the cohomology package? Well, they are to be installed into Sage's version of GAP respectively Singular. So, I should try to understand how database_gap manages to install stuff into Sage's GAP without inserting stuff into Sage's src tree.

comment:158 in reply to: ↑ 157 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

It is more like "some code snippets which can be used by some optional package that the user may install". So, I could understand that we wouldn't like to have these code snippets in the src tree. However, they are supposed to be used in Sage's GAP and Singular, and thus it makes sense to put them into src/sage/libs. That's the dilemma...

An important question is how exactly are these GAP/Singular files going to be used? For example, database_gap and gap_packages contain GAP packages which are installed in exactly the same way as GAP packages would be installed outside of Sage. They are not used by Sage directly, only by the GAP-in-Sage. Would that work for you?

comment:159 in reply to: ↑ 158 Changed 3 years ago by SimonKing

Replying to jdemeyer:

An important question is how exactly are these GAP/Singular files going to be used? For example, database_gap and gap_packages contain GAP packages which are installed in exactly the same way as GAP packages would be installed outside of Sage. They are not used by Sage directly, only by the GAP-in-Sage. Would that work for you?

The GAP functions currently are in files and are loaded into GAP like this:

        if not gap('IsBoundGlobal("exportMTXLIB")'):
            gap.eval('Read("%s/src/ext/gap/modular_cohomology/GapMaxels.g");'%(SAGE_ROOT))
            gap.eval('Read("%s/src/ext/gap/modular_cohomology/GapMB.g");'%(SAGE_ROOT))
            gap.eval('Read("%s/src/ext/gap/modular_cohomology/GapSgs.g");'%(SAGE_ROOT))
            gap.eval('InstallValue(exportMTXLIB,"MTXLIB=%s; export MTXLIB; ")'%(os.path.join(DOT_SAGE,"meataxe")))

The Singular functions are in interpreted libraries that are loaded like this:

        singular.load('{}/dickson.lib'.format(os.path.join(SAGE_ROOT,'src','ext','singular')))
        singular.load('{}/filterregular.lib'.format(os.path.join(SAGE_ROOT,'src','ext','singular')))

It would be OK from my point of view to copy filterregular.lib and dickson.lib into the shared folder containing all Singular libraries. Then, one could simply do

        singular.LIB('dickson.lib')
        singular.LIB('filterregular.lib')

I certainly could make the modular cohomology package insert stuff into SAGE_LOCAL/share/singular/LIB/. Actually I'd prefer this.

And it would also be OK to have some kind of GAP package, so that one could say

        gap.LoadPackage('WhateverThisPackageIsCalled')

However, I simply don't know how to create a Gap package, thus, I'd appreciate a pointer.

comment:160 follow-up: Changed 3 years ago by dimpase

have a look at https://github.com/gap-packages/example

(if your GAP code does not call external programs, this is an overkill

comment:161 in reply to: ↑ 160 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

have a look at https://github.com/gap-packages/example

Thank you for the pointer!

(if your GAP code does not call external programs, this is an overkill

Actually the GAP code is the only part of the group cohomology computation that calls external programs --- currently, for example, like that:

  Exec(Concatenation("mkdir -p ", dir));
...
  buffer := Concatenation(exportMTXLIB, "makeInclusionMatrix ", Gstem, " ", Hstem,
    " ", Istem);
  Print(buffer, "\n");
  Exec(buffer);

I somehow have the feeling that external programs (here: mkdir and !makeInclusionMatrix) shouldn't be called in that way. So, if gap_packages/example tells me how to do better, I'd be glad!

comment:162 in reply to: ↑ 161 ; follow-up: Changed 3 years ago by SimonKing

Replying to SimonKing:

(if your GAP code does not call external programs, this is an overkill

Actually the GAP code is the only part of the group cohomology computation that calls external programs

Maybe I misunderstood what you meant be "call external programs". In contrast to gap-packages/example, my gap package would simply consist of a bunch of gap-readable files, but it would not provide the c sources for the external programs it is calling.

Anyway, is Exec(...) still the way to call programs from within GAP?

comment:163 in reply to: ↑ 162 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to SimonKing:

(if your GAP code does not call external programs, this is an overkill

Actually the GAP code is the only part of the group cohomology computation that calls external programs

Maybe I misunderstood what you meant be "call external programs". In contrast to gap-packages/example, my gap package would simply consist of a bunch of gap-readable files, but it would not provide the c sources for the external programs it is calling.

is there a reason for not packaging them in? Are they called from anywhere else?

Anyway, is Exec(...) still the way to call programs from within GAP?

Yes. But I suppose calling mkdir the way you do is a bit rough. GAP has its own facilities for that, see DirectoryTemporary.

comment:164 in reply to: ↑ 163 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

Replying to SimonKing:

Maybe I misunderstood what you meant be "call external programs". In contrast to gap-packages/example, my gap package would simply consist of a bunch of gap-readable files, but it would not provide the c sources for the external programs it is calling.

is there a reason for not packaging them in? Are they called from anywhere else?

The executables are part of a C code base which also provides a library. My Cython code is linked against that library. So, no, the executables aren't called anywhere else, but, yes, the package which the executables belong to is used somewhere else.

Anyway, is Exec(...) still the way to call programs from within GAP?

Yes. But I suppose calling mkdir the way you do is a bit rough. GAP has its own facilities for that, see DirectoryTemporary.

The directory to be created is supposed to be permanent.

comment:165 in reply to: ↑ 164 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

Replying to SimonKing:

Maybe I misunderstood what you meant be "call external programs". In contrast to gap-packages/example, my gap package would simply consist of a bunch of gap-readable files, but it would not provide the c sources for the external programs it is calling.

is there a reason for not packaging them in? Are they called from anywhere else?

The executables are part of a C code base which also provides a library. My Cython code is linked against that library. So, no, the executables aren't called anywhere else, but, yes, the package which the executables belong to is used somewhere else.

OK, makes sense then. This means it's simpler, in the sense that one won't really need much configure/makefile stuff, if at all.

Anyway, is Exec(...) still the way to call programs from within GAP?

Yes. But I suppose calling mkdir the way you do is a bit rough. GAP has its own facilities for that, see DirectoryTemporary.

The directory to be created is supposed to be permanent.

This sounds like trouble. E.g. think about a multi-user situation... Does it mean to be some soft of caching? There is a package like this in GAP, AtlasRep. There data is fetched from the net, rather than computed, IIRC.

comment:166 in reply to: ↑ 165 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

The directory to be created is supposed to be permanent.

This sounds like trouble. E.g. think about a multi-user situation...

I thought of a multi-user situation.

Does it mean to be some sort of caching? There is a package like this in GAP, AtlasRep. There data is fetched from the net, rather than computed, IIRC.

It's kind of similar.

  • There is a repository in the web.
  • There is a local repository that all users of a Sage installation can read; I call this the public database. Of course, only a user with write privileges can add data to the public database.
  • Each user has his or her own local repository (I call it "private database"). The data in the private database is soft links to the public database (when the data is available there), otherwise is fetched from the web (when the data is available there), otherwise (or if explicitly requested) is computed from scratch.

In particular, in a multi-user setting, the users share the data from the public database. However, each user may individually compute further information on a cohomology ring, that is stored in his/her private database and won't interfere with the shared data.

comment:167 in reply to: ↑ 166 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

The directory to be created is supposed to be permanent.

This sounds like trouble. E.g. think about a multi-user situation...

I thought of a multi-user situation.

Does it mean to be some sort of caching? There is a package like this in GAP, AtlasRep. There data is fetched from the net, rather than computed, IIRC.

It's kind of similar.

  • There is a repository in the web.
  • There is a local repository that all users of a Sage installation can read; I call this the public database. Of course, only a user with write privileges can add data to the public database.
  • Each user has his or her own local repository (I call it "private database"). The data in the private database is soft links to the public database (when the data is available there), otherwise is fetched from the web (when the data is available there), otherwise (or if explicitly requested) is computed from scratch.

In particular, in a multi-user setting, the users share the data from the public database. However, each user may individually compute further information on a cohomology ring, that is stored in his/her private database and won't interfere with the shared data.

Then you perhaps can borrow their design. Surely Thomas Breuer, the main developer of AtlasRep, would be glad to help.

Last edited 3 years ago by dimpase (previous) (diff)

comment:168 in reply to: ↑ 167 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

In particular, in a multi-user setting, the users share the data from the public database. However, each user may individually compute further information on a cohomology ring, that is stored in his/her private database and won't interfere with the shared data.

Then you perhaps can borrow their design. Surely Thomas Breuer, the main developer of AtlasRep, would be glad to help.

On the other hand, the current question is not about the design of data storage/caching, but about the code layout. Is the AtlasRep package similar in that regard as well?

comment:169 in reply to: ↑ 168 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

In particular, in a multi-user setting, the users share the data from the public database. However, each user may individually compute further information on a cohomology ring, that is stored in his/her private database and won't interfere with the shared data.

Then you perhaps can borrow their design. Surely Thomas Breuer, the main developer of AtlasRep, would be glad to help.

On the other hand, the current question is not about the design of data storage/caching, but about the code layout. Is the AtlasRep package similar in that regard as well?

I haven't looked at its source for many years; it's reasonably well-documented, have a look.

comment:170 in reply to: ↑ 169 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

On the other hand, the current question is not about the design of data storage/caching, but about the code layout. Is the AtlasRep package similar in that regard as well?

I haven't looked at its source for many years; it's reasonably well-documented, have a look.

I just had a look, and apparently it is just a bunch of gap readable files. It seems that it contains no compiled code, in particular no linking against third party libraries.

comment:171 Changed 3 years ago by git

  • Commit changed from ae029297d600471d77f3de795c9ad8d726988a3b to 12e1bc50e4ef6ceedaaf30e66e1fd03d5d1cadf6

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

d4f8496Documentation of new functions. Fix doctest in cochain.pyx
282ae43Fixing almost all tests in cohomology.pyx. Change Simon King's address
99f2f3bRemove 'timing' option. Remove commented out old code. Fix tests in modular_cohomology.pyx
10ea380Remove 'shebang lines' from modular_resolution's spkg-* scripts
4bf8addConversion of matrix data into Matrix_t* moved from matrix_gfpn_dense to libs/meataxe
c032ac7Add libraries that the cohomology code is to be linked with
13482a1Remove everything but CohomologyRing from __init__.py
a73ef8fFix import locations and types in cochain.*
83ec3ef(modular_)cohomology.pyx: Fix import locations, cope with changes in matrix_gfpn_dense
12e1bc5resolution.pyx: Fix import locations, cope with changes in matrix_gfpn_dense, fix dict keys in LIFTContainer

comment:172 in reply to: ↑ 170 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

On the other hand, the current question is not about the design of data storage/caching, but about the code layout. Is the AtlasRep package similar in that regard as well?

I haven't looked at its source for many years; it's reasonably well-documented, have a look.

I just had a look, and apparently it is just a bunch of gap readable files. It seems that it contains no compiled code, in particular no linking against third party libraries.

Why do you need the latter? Don't you just call executables? They call wget.

comment:173 Changed 3 years ago by SimonKing

  • Description modified (diff)

I got the code running! Probably some tests need fixing (some log strings may have changed, I am confident that the mathematical content didn't). Also, I should apply what I recently learnt about sig_on/sig_off vs. sig_check.

To be on the safe side, I tried to post the latest modular_resolution-1.0.tar.gz, but apparently there is a problem on the webserver in Jena, and it is too large to post here. Anyway, IIRC, the file at http://users.minet.uni-jena.de/cohomology/modular_resolution-1.0.tar.gz is still the correct version (not 100% sure though).

It certainly isn't ready for a formal review. The main part of the code still consists of cython OptionalExtension, which should eventually be moved into a pip installable module. Also, the location of the GAP and Singular code files should be fixed.

In any case, it should now provide the possibility to do some testing. Installation:

  • checkout the branch
  • sage -i database_gap
  • sage -i meataxe
  • copy modular_resolution-1.0.tar.gz into SAGE_ROOT/upstream
  • sage -i modular_resolution
  • make

IIRC, Sage won't build the documentation of OptionalExtension. For the documentation of the old version of the package, see http://users.minet.uni-jena.de/cohomology/documentation/, but note that in the current shape of the code, from pGroupCohomology import ... needs to be replaced by from sage.groups.modular_cohomology import ....

comment:174 in reply to: ↑ 172 ; follow-ups: Changed 3 years ago by SimonKing

Replying to dimpase:

Why do you need the latter? Don't you just call executables? They call wget.

The meataxe package provides a library "mtx" and some executables.

The C code in modular_resolution-1.0 (which is based on David Green's code but for which I became upstream) needs to link against mtx. It provides a library "modres" and some executables.

The Cython OptionalExtensions that you'll currently find in sage/groups/modular_cohomology (but that will be moved into a pip installable module!) need to be linked both against mtx and against modres.

I am not calling the meataxe executables directly. However, IIRC, the executables provided by modular_resolution-1.0 do call some meataxe executables.

At several stages of the cohomology computation, I need to call the modular_resolution executables; it results in many data files stored on disk in special folders, all data together determine an initial segment of the minimal projective resolution of the group algebra, and the data is then used by Cython code to compute the ring structure of the cohomology ring.

The calls to modular_resolution executables partially occur in GAP functions, and GAP functions are also responsible to create the special folders. However, the path of the special folders and also the names of the data files are determined by the Cython code. So, it should (when needed) be possible to move all the calls from GAP code to Cython code.

Concerning wget: It is the responsibility of my Python code to check whether the data of a cohomology ring are found locally. If data is to be fetched from web, which is done with Python's urllib2 module. I guess urllib2 uses wget, but I am not using wget directly.

I hope that clarified the structure of the code base.

comment:175 Changed 3 years ago by SimonKing

PS: Perhaps I should tell why the data of a single cohomology ring are distributed in dozens of data files.

When developing the very first version of the group cohomology spkg, I tried to turn the modular_resolution executables into library functions that simply create the resolution data in memory, rather then storing them on disk. But I soon ran out of memory. So, I kept the executables and use the data files.

The cohomology computation is in increasing degree, and I originally tried to keep the cohomology data of lower degrees in memory when computing higher degrees. But I soon ran out of memory again. So, I started to cache data on disk.

Originally, I tried to pickle the cohomology data into one single file (plus all the files constituting the resolution data). However, in some examples, the process of pickling a cohomology ring took hours --- simply the data was too large. Hence, I started to dump cohomology data in several smaller files, basically one in each degree, and then some more defining the ring maps induced by inclusion of certain special subgroups.

I hope that explains why I care about how data are stored. I am sure that the computation of the mod-2 cohomology of the third Conway group or of all groups of order 128 would have been hardly possible when using a simple "one group, one file" approach to store data. Instead, it is "one group, one folder".

comment:176 in reply to: ↑ 174 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

The meataxe package provides a library "mtx" and some executables.

The C code in modular_resolution-1.0 (which is based on David Green's code but for which I became upstream) needs to link against mtx. It provides a library "modres" and some executables.

The Cython OptionalExtensions that you'll currently find in sage/groups/modular_cohomology (but that will be moved into a pip installable module!) need to be linked both against mtx and against modres.

Is meataxe still a static library? I remember that we had some discussion about that. If you link from different libraries or Python modules to meataxe, every instance will get an independent copy of meataxe. This is generally considered to be bad practice. For example, I remember that meataxe uses some kind of data directory. That needs to be set for every individual copy of the meataxe static library. With a dynamic library on the other hand, a program uses only one copy of meataxe. Setting the data directory once should then be sufficient.

comment:177 in reply to: ↑ 174 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

The Cython OptionalExtensions that you'll currently find in sage/groups/modular_cohomology (but that will be moved into a pip installable module!)

If you plan to move it to a separate package, why take the detour via Sage? Maybe you could do it just for testing purposes, that might make sense. However, it seems like you are making things difficult for yourself if you first want your code to be merged in Sage and then taken out again as a separate package.

comment:178 in reply to: ↑ 176 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Is meataxe still a static library? I remember that we had some discussion about that. If you link from different libraries or Python modules to meataxe, every instance will get an independent copy of meataxe. This is generally considered to be bad practice. For example, I remember that meataxe uses some kind of data directory. That needs to be set for every individual copy of the meataxe static library. With a dynamic library on the other hand, a program uses only one copy of meataxe. Setting the data directory once should then be sufficient.

I totally agree with that, but recall that I am not upstream for MeatAxe (I only contributed patches to the current MeatAxe spkg). Hence, unless someone tells me how to easily change MeatAxe's Makefile, it will remain static. However, if there is an easy way to change things, I'll be happy to include yet another patch to the MeatAxe spkg.

comment:179 in reply to: ↑ 177 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

The Cython OptionalExtensions that you'll currently find in sage/groups/modular_cohomology (but that will be moved into a pip installable module!)

If you plan to move it to a separate package, why take the detour via Sage? Maybe you could do it just for testing purposes, that might make sense.

Exactly, that was my reason.

However, it seems like you are making things difficult for yourself if you first want your code to be merged in Sage and then taken out again as a separate package.

That's why I did not change this ticket to "needs review" in the last couple of years.

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

comment:180 follow-up: Changed 3 years ago by dimpase

If database_gap is a dependency of meataxe (it seems so from the commands above) then it should be in build/pkgs/meataxe/dependencies.

Same commentary for build/pkgs/modular_resolution/dependencies - it should mention meataxe.

Last edited 3 years ago by dimpase (previous) (diff)

comment:181 in reply to: ↑ 180 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

If database_gap is a dependency of meataxe (it seems so from the commands above) then it should be in build/pkgs/meataxe/dependencies.

It isn't.

database_gap is a run-time dependency for the cohomology package (in the sense that it provides the SmallGroups library), but it has nothing to do with MeatAxe or with modular_resolution.

comment:182 in reply to: ↑ 181 ; follow-ups: Changed 3 years ago by dimpase

Replying to SimonKing:

Replying to dimpase:

If database_gap is a dependency of meataxe (it seems so from the commands above) then it should be in build/pkgs/meataxe/dependencies.

It isn't.

database_gap is a run-time dependency for the cohomology package (in the sense that it provides the SmallGroups library), but it has nothing to do with MeatAxe or with modular_resolution.

run-time dependency is a dependency too.


OK, I see, you are saying that installing modular_decomposition should trigger installation of meataxe and database_gap (the format of dependencies there is weird, I am not sure these $(INST)/ are the right thing - most if not all packages don't have it`)

Last edited 3 years ago by dimpase (previous) (diff)

comment:183 in reply to: ↑ 182 Changed 3 years ago by SimonKing

Replying to dimpase:

database_gap is a run-time dependency for the cohomology package (in the sense that it provides the SmallGroups library), but it has nothing to do with MeatAxe or with modular_resolution.

run-time dependency is a dependency too.

I can only repeat myself. database_gap has NOTHING TO DO WHATSOEVER with MeatAxe.

So, if anything, we are talking here about a runtime dependency for what currently is (but soon will not be) OptionalExtension in sage/groups/modular_cohomology/. IIRC, I was told that the dependencies listed in OptionalExtension only concern build time dependencies (which is mtx and modres), but not runtime dependencies.

comment:184 follow-up: Changed 3 years ago by dimpase

files in src/sage/groups/modular_cohomology/ are not python3-ready (all these print blah instead of print(blah)), this causes havoc while attempting to run tests, it seems. E.g.

       print CohomologyRing(Integer(64),Integer(12))  # indirect doctest
                           ^
    SyntaxError: invalid syntax

comment:185 in reply to: ↑ 182 Changed 3 years ago by SimonKing

Replying to dimpase:

OK, I see, you are saying that installing modular_decomposition [ I guess you mean modular_resolution ] should trigger installation of meataxe and database_gap (the format of dependencies there is weird, I am not sure these $(INST)/ are the right thing - most if not all packages don't have it`)

No, an automatic installation (without explicit consent of the user) of database_gap is not allowed (licence issue) and not proposed.

Also, there are various ways to install the SmallGroups library. So, I think the correct way to deal with the problem is:

  • meataxe clearly has no dependency whatsoever.
  • modular_resolution will still have the dependency meataxe, but no other dependency. I just checked: There is no use of SmallGroup in the whole code.
  • take the current experimental OptionalExtension from sage/groups/modular_cohomology and put them into a pip installable upstream tarball modular_group_cohomology.
  • build/pkgs/modular_group_cohomology/dependencies is, as far as I understand, for build time dependencies, thus, it will only list meataxe and modular_resolution, but not database_gap.
  • build/pkgs/spkg-install will check that the SmallGroups library is in fact installed (it doesn't matter if it comes from database_gap or another source). If it isn't it will fail and advise the user to install it one way or another.
Last edited 3 years ago by SimonKing (previous) (diff)

comment:186 follow-up: Changed 3 years ago by dimpase

there are also some import errors, like

Running doctests with ID 2017-12-10-09-59-37-95694ca7.
Git branch: cohomolo
Using --optional=4ti2,cbc,ccache,cmake,database_gap,dot2tex,gambit,gap_packages,latte_int,libhomfly,libogg,lidia,lrslib,meataxe,modular_resolution,mpir,normaliz,pynormaliz,python2,qhull,sage,scons
Doctesting 1 file using 4 threads.
sage -t --warn-long 68.7 barcode.py
**********************************************************************
File "barcode.py", line 99, in sage.groups.modular_cohomology.barcode
Failed example:
    B158 = H158.bar_code('UpperCentralSeries')
Exception raised:
    Traceback (most recent call last):
      File "/home/dima/Sage/sage-dev/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dima/Sage/sage-dev/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 885, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.modular_cohomology.barcode[18]>", line 1, in <module>
        B158 = H158.bar_code('UpperCentralSeries')
      File "sage/groups/modular_cohomology/cohomology.pyx", line 1673, in sage.groups.modular_cohomology.cohomology.permanent_result.__call__ (build/cythonized/sage/groups/modular_cohomology/cohomology.c:28178)
        val = self._f(inst,*args,**kwds)
      File "sage/groups/modular_cohomology/cohomology.pyx", line 11034, in sage.groups.modular_cohomology.cohomology.COHO.bar_code (build/cythonized/sage/groups/modular_cohomology/cohomology.c:156393)
        OUT[i,j] = C[j].hom(GH[i,j],C[i]).poincare_of_image()
      File "sage/groups/modular_cohomology/cochain.pyx", line 7054, in sage.groups.modular_cohomology.cochain.ChMap.poincare_of_image (build/cythonized/sage/groups/modular_cohomology/cochain.c:76428)
        from sage.groups.modular_cohomology import singular
    ImportError: cannot import name singular

comment:187 in reply to: ↑ 184 Changed 3 years ago by SimonKing

Replying to dimpase:

files in src/sage/groups/modular_cohomology/ are not python3-ready (all these print blah instead of print(blah)), this causes havoc while attempting to run tests, it seems. E.g.

       print CohomologyRing(Integer(64),Integer(12))  # indirect doctest
                           ^
    SyntaxError: invalid syntax

OMG, I thought the old syntax is still fine in tests. Sure, I need to change all print statements in the tests, then.

comment:188 in reply to: ↑ 186 Changed 3 years ago by SimonKing

Replying to dimpase:

there are also some import errors, like

    ImportError: cannot import name singular

Thanks for finding this. I did do some computations in interactive sessions, but didn't run the test suite yet.

The point is that all the modules involved in the computation are supposed to use the same singular instance. And apparently I have changed something.

While we are at it: Is it OK to just import singular from sage.interfaces.singular? Or would you think it makes sense to provide a hook so that a user can plug in his/her own singular interface to the computation? I guess the former is easier and the latter doesn't add an advantage.

comment:189 Changed 3 years ago by SimonKing

I created #24359 to change MeatAxe into a dynamic library.

comment:190 Changed 3 years ago by SimonKing

  • Dependencies changed from #21437 to #24359

comment:191 Changed 3 years ago by SimonKing

After dealing with #24359, I'm back at the cohomology package.

There now remains to deal with two things:

  • C code providing a library libmodres and some binaries; it is autotoolized and links against libmtx.
  • Python and Cython code linked against libmtx and libmodres and is NOT standalone as it makes use of Sage types and Sage infrastructure.

How would you recommend to proceed?

  1. Have two separate packages modular_resolution (installing libmodres) and p_group_cohomology (pip-installing the Python/Cython? modules) that the user has to install both?
  2. Have a single package that both installs libmodres and the Python/Cython? code?
  3. Something else?

I somehow tend towards 2., since libmodres and the executables hardly are of any use without the Cython and Python code. But I can also see the advantage of a strict modularisation, as in 1.

comment:192 Changed 3 years ago by SimonKing

Also, I need some pointers on how to create a pip installable module.

  • Somewhere I found the information that both setup.py and Manifest.in are required for pip installation. However, in some pip installable packages in SAGE_ROOT/upstream I do not find Manifest.in. So, what's the matter with it? If setup.py is enough, is there any advantage of pip install over python setup.py install?
  • In the old spkg, I was using distutils in setup.py. But somewhere I found the information that pip uninstall won't work with distutils. But I reckon that in future Sage will support uninstalling spkgs. So, should I use setuptools instead? Would that be able to build Cython code as well?

comment:193 follow-up: Changed 3 years ago by SimonKing

The third topic for which I need pointers: Documentation.

When I was young, Sage's documentation used a script builder.py to build its documentation. In the first versions of the cohomology spkg, I copied and modified it. Now, I see that builder.py has gone from Sage.

The question is how I could most easily build a reference manual for my spkg from all the docstrings in Cython and Python.

  • Somewhere I found a blog post of Jeroen about "sphinx and Cython", advising to compile the Cython code with binding=True. Is that currently done in Sage? Wouldn't that involve a mild slow-down?
  • My impression is that using Sphinx with Cython is currently done in conf.py by replacing inspect.getargspec() by some custom version of getargspec() that works on Cython code. Would that already be enough? I could easily provide such a function, by creating a modified copy sage.misc.sageinspect or perhaps even by monkey-patching it.
  • Would it be worth-while to exploit Sage's doc build infrastructure? Say, by overriding SAGE_DOC and SAGE_DOC_SRC in sage_setup.docbuild.__init__? Does that have a chance to work?

comment:194 Changed 3 years ago by SimonKing

  • Dependencies changed from #24359 to #24359 #24468

comment:195 in reply to: ↑ 193 Changed 3 years ago by SimonKing

For the record:

Replying to SimonKing:

  • My impression is that using Sphinx with Cython is currently done in conf.py by replacing inspect.getargspec() by some custom version of getargspec() that works on Cython code. Would that already be enough? I could easily provide such a function, by creating a modified copy sage.misc.sageinspect or perhaps even by monkey-patching it.

This works. At least when using compiler_directives={'embedsignature': True} and then adding some helper function to doc/sources/conf.py that removes the signature from the docstring before passing it to sphinx.

comment:196 Changed 2 years ago by git

  • Commit changed from 12e1bc50e4ef6ceedaaf30e66e1fd03d5d1cadf6 to 5e011619bc4814aeb02f785e327fb7ecff8eda1f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8d26842Upgrade meataxe spkg to SharedMeatAxe 1.0 (dynamic library)
13828d1Various fixes to spkg-install
1bc5815Use compatible implementation for MatrixSpace in mtx_unpickle
09ef3e0Merge branch 'u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces' of git://trac.sagemath.org/sage into coho_rebase_base
2ac7e04More stuff in the meataxe interface, and a meataxe helper function
fbc779eExpose matrix_gfpn_dense.FieldConverter to the interface
17787a1Fix reading MeatAxe matrix from a file
5e01161Add p_group_cohomology-3.0 spkg

comment:197 Changed 2 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:198 Changed 2 years ago by SimonKing

  • Description modified (diff)
Note: See TracTickets for help on using tickets.