Opened 5 years ago

Last modified 2 years ago

#18514 closed defect

Upgrade of group cohomology spkg — at Version 31

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: Commit:
Dependencies: #18494 Stopgaps:

Description (last modified by 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.

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

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

Change History (31)

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 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 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)
Note: See TracTickets for help on using tickets.