Upgrade of p_group_cohomology spkg

simon-king-jena commented 9 years ago

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

https://users.fmi.uni-jena.de/cohomology/p_group_cohomology-3.0.tar.xz

Upstream: None of the above - read trac for reasoning.

CC: @jhpalmieri @vbraun @jdemeyer @haraldschilly @frederichan-IMJPRG david.green@uni-jena.de

Component: packages: optional

Keywords: group cohomology

Author: Simon King

Branch: 6576915

Reviewer: Travis Scrimshaw, Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/18514

simon-king-jena commented 9 years ago

comment:96

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

jdemeyer commented 9 years ago

Changed dependencies from #18494 to #12103

simon-king-jena commented 9 years ago

comment:98

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.

simon-king-jena commented 9 years ago

comment:99

Replying to @simon-king-jena:

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?

dimpase commented 9 years ago

comment:100

Replying to @simon-king-jena:

Replying to @simon-king-jena:

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.

simon-king-jena commented 9 years ago

comment:101

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?

simon-king-jena commented 9 years ago

comment:102

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

dimpase commented 9 years ago

comment:103

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

simon-king-jena commented 9 years ago

comment:104

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.

simon-king-jena commented 9 years ago

comment:105

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?

simon-king-jena commented 9 years ago

comment:106

Replying to @simon-king-jena:

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

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

vbraun commented 9 years ago

comment:107

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ad6cea0 to 499dfbd

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

499dfbd Add package modular_resolution

simon-king-jena commented 9 years ago

comment:109

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()

simon-king-jena commented 9 years ago

comment:110

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)?

simon-king-jena commented 9 years ago

comment:111

libmodres_a_LDFLAGS = -static?

simon-king-jena commented 9 years ago

comment:112

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.

vbraun commented 9 years ago

comment:113

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?

simon-king-jena commented 9 years ago

comment:114

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!

simon-king-jena commented 9 years ago

comment:115

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?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

cd644a3 Add package modular_resolution

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 499dfbd to cd644a3

simon-king-jena commented 9 years ago

comment:117

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.

simon-king-jena commented 9 years ago

comment:118

Replying to @simon-king-jena:

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?

jdemeyer commented 9 years ago

comment:119

Replying to @simon-king-jena:

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?

simon-king-jena commented 9 years ago

comment:120

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.

jdemeyer commented 9 years ago

comment:121

Replying to @simon-king-jena:

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.

simon-king-jena commented 9 years ago

comment:122

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?

simon-king-jena commented 9 years ago

comment:123

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

simon-king-jena commented 9 years ago

comment:124

Replying to @simon-king-jena:

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?

simon-king-jena commented 9 years ago

comment:125

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from cd644a3 to 9999966

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

9999966 Add package modular_resolution

simon-king-jena commented 9 years ago

comment:127

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.

simon-king-jena commented 9 years ago

comment:128

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?

jdemeyer commented 9 years ago

comment:129

Replying to @simon-king-jena:

Singular code?

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

simon-king-jena commented 9 years ago

comment:130

Replying to @jdemeyer:

Replying to @simon-king-jena:

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.

simon-king-jena commented 9 years ago

comment:131

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

dimpase commented 9 years ago

comment:132

Replying to @simon-king-jena:

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

simon-king-jena commented 9 years ago

Changed work issues from Create a new-style package with least effort to Debug the modification of the modular resolution code

simon-king-jena commented 9 years ago

comment:134

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.

simon-king-jena commented 8 years ago

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 @simon-king-jena:

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.

simon-king-jena commented 8 years ago

Changed work issues from Debug the modification of the modular resolution code to none

simon-king-jena commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,21 +1,16 @@
-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.
+The current "official" version of `p_group_cohomology` is 2.1.4. However, due to recent backward incompatible changes in Sage, the package would not install, respectively it would install if some header were present but wouldn't work. Hence, an upgrade is needed.

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

-There is an old-style spkg at http://users.minet.uni-jena.de/cohomology/p_group_cohomology-2.1.5.spkg that users can install if they want to do cohomology calculations *now*. However, I suggest to a version 3.0 of the package, which shall be a new-style package.
+There is an old-style spkg at http://users.minet.uni-jena.de/cohomology/p_group_cohomology-2.1.6.spkg that users can install if they want to do cohomology calculations *now*. However, I suggest to a version 3.0 of the package, which shall be a new-style package.

-The new package shall be modularised as follows.
+The old-style package comprises three code bases: An old meataxe version, C code of David Green that uses a custom makefile, and P/Cython/Gap/Singular code equipped with a doc builder and a large-ish doc tester that executes each test in a separate Sage session, rather than running all tests of one file in one session (in order to prevent side-effects).

-- First, install meataxe (see #12103) and database_gap.
-- Second, get the branch from here.
-- Third, get the sources for modular_resolution-1.0. It is based on code of David Green, which I refactored rather extensively:
-  * It now uses the optional meataxe-2.4.24 package, rather than meataxe-2.2.4 whose sources used to be part of the old spkg.
-  * It propagates errors (at least to some extent). The original version would just crash if a file was missing.
-  * It is autotoolized.
+The group cohomology spkg will be refactored as follows:

-I am upstream, and you find the tarball at http://users.minet.uni-jena.de/cohomology/ (To be precise: http://users.minet.uni-jena.de/cohomology/modular_resolution-1.0.tar.bz2).
-
-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.
-
+- It depends on the optional meataxe package (see #12103), which is based on the most recent upstream version available in Aachen and is of independent use in Sage (as it speeds up matrix arithmetic over some fields).
+- Its first component is a refactored version of David Green's code, which is based on the new optional meataxe package, does proper error propagation, and is built using autotools.
+- Its second component is all the Python/Cython/Gap/Singular code, that spkg-install installs the pip way.
+- 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.
+- In some way (perhaps using google-style docstrings) the docs are built.

simon-king-jena commented 8 years ago

Changed commit from 9999966 to none

simon-king-jena commented 8 years ago

Changed branch from u/SimonKing/upgrade_of_group_cohomology_spkg to none

simon-king-jena commented 8 years ago

comment:137

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:

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).
Please point me to a good example of a new-style spkg that teaches me how to build a nice documentation based on docstrings.
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.

simon-king-jena commented 8 years ago

Changed dependencies from #12103 to #12103 #21437

simon-king-jena commented 8 years ago

comment:138

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

sagemath / sage

Upgrade of p_group_cohomology spkg #18514