Upgrade p_group_cohomology to version 3.1

[simon-king-jena]

A recent fix in Singular makes computing a certain invariant of modular cohomology rings of finite groups faster, but another recent fix in Singular makes the p_group_cohomology package fail.

The new version p_group_cohomology-3.0.1 is supposed to cope with the changes in Singular. I suggest to base it on top of #24735 and make it a dependency for #25993 (which contains the above mentioned changes in Singular).

While I was at it, I also improved more features of the package. Hence, I propose to upgrade to p_group_cohomology-3.1.

Upstream tarball v3.1

Depends on #24735
Depends on #22626
Depends on #26389
Depends on #26856

CC: @jdemeyer @tscrim @vbraun

Component: packages: optional

Keywords: group cohomology singular

Author: Simon King

Branch/Commit: 076cbf2

Reviewer: Travis Scrimshaw, Dima Pasechnik

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

[simon-king-jena]

Dependencies: #24735

[simon-king-jena]

comment:2

For the record: I fixed one problem resulting from a change in Singular, but apparently there are more to come. And actually the problems already seem to be caused by #24735, not only #25993.

[jdemeyer]

comment:3

Replying to @simon-king-jena:

For the record: I fixed one problem resulting from a change in Singular, but apparently there are more to come.

I ran the Sage testsuite with #24735 and didn't get any failures. But maybe you are talking about issues in p_group_cohomology which do not affect Sage doctests?

[simon-king-jena]

comment:4

Replying to @jdemeyer:

Replying to @simon-king-jena:

For the record: I fixed one problem resulting from a change in Singular, but apparently there are more to come.

I ran the Sage testsuite with #24735 and didn't get any failures. But maybe you are talking about issues in p_group_cohomology which do not affect Sage doctests?

Right, it seems that this one had a different reason.

[simon-king-jena]

Branch: u/SimonKing/upgrade_p_group_cohomology_to_version_3_0_1

[simon-king-jena]

comment:7

The new version copes with the changes in Singular also adds a new method that allows to find filter regular parameters in very small degrees after the computation of the cohomology ring has been completed. This may be useful to verify a conjecture of Benson in some open case.

Known problem

In some Singular procedures, I use a Hilbert driven computation, which in older versions of Singular used to be a lot faster and less buggy than what Singular could offer (this is in the computation of intersections, quotients etc.). In the new p_group_cohomology version, I found that in one example, my Hilbert driven procedures would give a wrong result. So, I changed the procedures, using Singular's built-in functions.

Now, all tests pass. However, some of these tests need a very long time, htop showing that the time is spent in Singular. Thus, I suppose it is because of dropping the Hilbert driven computations.

So, for now I am changing the ticket to "needs_review" --- after all, the tests pass. Nonetheless, I am investigating whether the slowness persists with Singular-4.1.1.p3 (after all, p3 fixes a performance problem in std(id1,id2) that was uncovered by my cohomology computations). If it doesn't, I will see how to work around the problem (such as: Use Hilbert only in characteristic two).

[simon-king-jena]

Author: Simon King

[simon-king-jena]

Commit: 0c40f3e

[simon-king-jena]

comment:8

It seems that there is another problem: Documentation. It seems that all pages except index.html are basically empty. Could a reviewer please have a look what is going wrong there that didn't go wrong before?

[simon-king-jena]

comment:9

With Singular-4.1.1.p3, one test fails. Again, it is related with std(id1,id2): In the old version, the Krull dimension of an ideal in some example was 5, in the new version it is 4.

But actually that could be my own fault: The ideal id1 is supposed to be a Gröbner basis, but in my example it isn't. So, this I need to change.

Unfortunately, the "long time" problem persists in Singular-4.1.1.p3. So, I have to return to using my Hilbert driven computations.

[simon-king-jena]

Work Issues: Work around Singular bugs and slowness

[simon-king-jena]

comment:11

Can the documentation problem be caused by having a wrong version number in the conf.py?? I couldn't test yet whether building the docs during package installation works. But building the docs in a separate directory does work.

[simon-king-jena]

comment:12

Not good at all.

• Although the documentation builds fine when I build it separately, it consists of basically empty pages when I build it during package installation.
• Some doctests are very slow, because of Singular, regardless whether it is Singular-4.1.1.p2 or .p3; at least it seems that it is not caused by my Hilbert driven computations. It could actually be that the slowness is in a function that could use a Hilbert driven computation, but currently doesn't. I need to investigate.

[simon-king-jena]

comment:13

Concerning the timings, I am not sure what is happening. As part of the test suite, I get

[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 5178, in pGroupCohomology.cohomology.COHO.essential_ideal
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     I = H.essential_ideal()    #long time
[p_group_cohomology-3.0.1] Test ran for 60.72 s

In an interactive session, the same test becomes

CPU times: user 5.26 s, sys: 1.03 s, total: 6.29 s
Wall time: 36.3 s

which is the expected long times, but not as long as during the tests.

[simon-king-jena]

comment:14

Even more extreme is that one:

[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 2391, in pGroupCohomology
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     H.make(14)
[p_group_cohomology-3.0.1] Test ran for 892.58 s

versus

sage: %time H.make(14)                                            
CPU times: user 7.32 s, sys: 1.52 s, total: 8.84 s
Wall time: 14.1 s

Could the problem lie in Sage's doctest framework? Are there known issues, maybe related with pexpect (which the computations heavily rely on)

[tscrim]

comment:15

Are these computations being run in parallel? Sage doctesting (done in parallel) can get into some thrashing issues when tests use parallel code not handled through Sage (I noticed similar behavior for some of my tests, which farmed out things to normaliz and ran code in parallel).

[simon-king-jena]

comment:16

Replying to @tscrim:

Are these computations being run in parallel? Sage doctesting (done in parallel) can get into some thrashing issues when tests use parallel code not handled through Sage (I noticed similar behavior for some of my tests, which farmed out things to normaliz and ran code in parallel).

I am not aware that the code uses threading. But that is maybe worth asking Hans Schönemann: It may be a possibility that the latest version of Singular uses parallel computations.

[jdemeyer]

comment:17

I don't think it's the doctest framework. Maybe the earlier doctests do something causing a slowdown later?

[jdemeyer]

comment:18

Replying to @tscrim:

Sage doctesting (done in parallel)

That's an important point: do you get the problems with doctesting in parallel or also when doctesting a single file?

[tscrim]

comment:19

Replying to @jdemeyer:

Replying to @tscrim:

Sage doctesting (done in parallel)

That's an important point: do you get the problems with doctesting in parallel or also when doctesting a single file?

If that is a question to me, in my case, it was whenever I did sage -tp. See this sage-devel thread.

[simon-king-jena]

comment:20

From spgk-check:

# testing pGroupCohomology
sage -tp 0 --long --force_lib --timeout=0 pyxsources/pGroupCohomology/ || sdh_die "Error testing pGroupCohomology"

I don't recall what -tp 0 does. As many processes as available, resp. as specified by MAKE="make -jN"?

I didn't try without -tp 0 yet, but will soon.

[tscrim]

comment:21

When running sage -btp 0, it uses all my cpus; so I am guessing it is equivalent to not passing any number. However, I am tempted to say it should run forever since it is testing something by using 0 cores. :P

[simon-king-jena]

comment:62

The merge conflict has only been in the "dependencies" file: #26856 changed it so that gap is a dependency, I changed it so that gap isn't mentioned. I hope it is fine to take the "dependencies" version of #26856.

Am I right that it needs another review? I am rebuilding Sage (with the latest develop) right now and will then try to rebuild and test the package.

[dimpase]

comment:63

I hate to think what could happen if a ticket merged in a beta-release stage gets unmerged before the release, while its branch is gone...

Anyway, I merely illustrated a basic two-liner to check out a named branch using plain git, no more than that.

---

New commits:

2c99aca

Merge branch 'develop' into t/26001/upgrade_p_group_cohomology_to_version_3_1

[dimpase]

comment:64

Replying to @simon-king-jena:

The merge conflict has only been in the "dependencies" file: #26856 changed it so that gap is a dependency, I changed it so that gap isn't mentioned. I hope it is fine to take the "dependencies" version of #26856.

I think the deps of an optional package ought to be as explicit as ones of a standard one, after all nothing prevents a user to try to install an optional package on an incomplete build of Sage, and then omitting deps would be lethal. So gap is there by right...

Am I right that it needs another review? I am rebuilding Sage (with the latest develop) right now and will then try to rebuild and test the package.

I think you could run the package's tests and mark the ticket positively reviewed if everything is OK.

[simon-king-jena]

comment:65

Sigh. It happened again. By some change, the package now fails completely. One cannot even import CohomologyRing. I have yet to find out what is happening there.

[simon-king-jena]

Work Issues: Cope with a downstream change

[simon-king-jena]

comment:66

sage: from pGroupCohomology import CohomologyRing
Traceback (most recent call last):
...
RuntimeError: Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:75
  if isPrimePower(Size(G)) then
                 ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:76
     RegAct := Group(verifiedMinGens(regularPermutationAction(G: forceDefiningGenerators)));
                                    ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:76
     RegAct := Group(verifiedMinGens(regularPermutationAction(G: forceDefiningGenerators)));
                                                             ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:78
     RegAct := regularPermutationAction(G: forceDefiningGenerators);
                                       ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:393
  Exec(Concatenation(exportMTXLIB, "makeActionMatrices ", name));
                                 ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:418
  Exec(Concatenation(exportMTXLIB, "makeNontips -O ", orderString, "  ",
                                 ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:517
    Exec(Concatenation(exportMTXLIB, "makeActionMatrices ", name));
                                   ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:548
  Exec(Concatenation(exportMTXLIB, "makeNontips -OJ ", String(ThePrime(ngg)), " ", name));
                                 ^
Syntax warning: Unbound global variable in /home/king/Sage/git/sage/local/share/sage/ext/gap/modular_cohomology/GapMB.g:549
  Exec(Concatenation(exportMTXLIB, "makeActionMatrices ", name));
                                 ^
Error, Variable: 'GroupName' is read only

[dimpase]

comment:67

This is GAP (one of its packages, I guess) that is using GroupName now:

$ ./sage --gap
 ┌───────┐   GAP 4.10.0 of 01-Nov-2018
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64
 Configuration:  gmp 6.0.0, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.1, Alnuth 3.1.0, AtlasRep 1.5.1, AutoDoc 2018.09.20, 
             AutPGrp 1.10, CRISP 1.4.4, Cryst 4.1.18, CrystCat 1.1.8, 
             CTblLib 1.2.2, FactInt 1.6.2, FGA 1.4.0, GAPDoc 1.6.2, IO 4.5.4, 
             IRREDSOL 1.4, LAGUNA 3.9.0, Polenta 1.3.8, Polycyclic 2.14, 
             PrimGrp 3.3.2, RadiRoot 2.8, ResClasses 4.7.1, SmallGrp 1.3, 
             Sophus 1.24, TomLib 1.2.7, TransGrp 2.0.4, utils 0.59
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> GroupName:=1;
Error, Variable: 'GroupName' is read only
not in any function at *stdin*:1
you can 'return;' after making it writable
brk> 

user GAP code should not use such generic variable names, as GAP has no proper namespaces---whereas you have

GroupName := function(Gid)
  local Gname, Gsize;
  Gsize := Gid[1];
  Gname := Concatenation(String(Gsize), "gp", String(Gid[2]));
  return Gname;
end;

in pyxsources/pGroupCohomology/GapSgs.g. So renaming this function would do...

[simon-king-jena]

comment:68

Replying to @dimpase:

So renaming this function would do...

OK. GroupName is a bad description of that function anyway. It takes a SmallGroup address (otherwise named Gid), say, (128,160), and turns it not into the name of the group (which would be "Modular group of order 128", but into the string "128gp160" (otherwise named GStem), which is used to create the names of data directories associated with that group.

So, I'll call it Gid2GStem.

[dimpase]

comment:69

I actually checked that with such a change (editing one GAP source file in like 4 places) the import above works.
(As you don't host your package publicly, I can't send you a pull request :-P)

[sagetrac-git]

Changed commit from 2c99aca to 076cbf2

[sagetrac-git]

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

076cbf2

p_group_cohomology: Avoid conflict with a new global variable name in Gap

[simon-king-jena]

comment:71

I have updated the tarball (same address as before). After the change, I get

[p_group_cohomology-3.1] ----------------------------------------------------------------------
[p_group_cohomology-3.1] All tests passed!
[p_group_cohomology-3.1] ----------------------------------------------------------------------
[p_group_cohomology-3.1] Total time for all tests: 467.4 seconds
[p_group_cohomology-3.1]     cpu time: 920.5 seconds
[p_group_cohomology-3.1]     cumulative wall time: 1350.9 seconds

Since the change hasn't been totally trivial, I leave it up to decide whether it can be "positive review".

[simon-king-jena]

Changed work issues from Cope with a downstream change to none

[dimpase]

comment:73

Happy New Year, Simon :-)

[dimpase]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Dima Pasechnik

[simon-king-jena]

comment:74

Replying to @dimpase:

Happy New Year, Simon :-)

To you as well, Dima!

[jdemeyer]

comment:75

Since the old package no longer works with Sage 8.6.rc0, this should be merged in 8.6

[vbraun]

Changed branch from u/SimonKing/upgrade_p_group_cohomology_to_version_3_1 to 076cbf2