#26001 closed defect (fixed)

Upgrade p_group_cohomology to version 3.1

Reported by: SimonKing Owned by:
Priority: blocker Milestone: sage-8.6
Component: packages: optional Keywords: group cohomology singular
Cc: jdemeyer, tscrim, vbraun Merged in:
Authors: Simon King Reviewers: Travis Scrimshaw, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 076cbf2 (Commits) Commit: 076cbf20c8ac18a1ef3d3d9c364f8dcc299f7ae2
Dependencies: #24735 #22626 #26389 #26856 Stopgaps:

Description (last modified by SimonKing)

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

Change History (76)

comment:1 Changed 20 months ago by SimonKing

  • Dependencies set to #24735

comment:2 follow-up: Changed 20 months ago by SimonKing

  • Cc jdemeyer added

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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 20 months ago by jdemeyer

Replying to SimonKing:

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?

comment:4 in reply to: ↑ 3 Changed 20 months ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

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.

comment:5 Changed 20 months ago by tscrim

  • Cc tscrim added

comment:6 Changed 20 months ago by SimonKing

  • Branch set to u/SimonKing/upgrade_p_group_cohomology_to_version_3_0_1

comment:7 Changed 20 months ago by SimonKing

  • Authors set to Simon King
  • Commit set to 0c40f3efc06cbc01f946cb6704090a704cf64a02
  • Description modified (diff)
  • Status changed from new to needs_review

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

comment:8 Changed 20 months ago by SimonKing

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?

comment:9 Changed 20 months ago by SimonKing

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.

comment:10 Changed 20 months ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Work around Singular bugs and slowness

comment:11 Changed 20 months ago by SimonKing

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.

comment:12 Changed 20 months ago by SimonKing

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.

comment:13 Changed 20 months ago by SimonKing

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.

comment:14 Changed 20 months ago by SimonKing

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)

comment:15 follow-ups: Changed 20 months ago by 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).

comment:16 in reply to: ↑ 15 Changed 20 months ago by SimonKing

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.

comment:17 Changed 20 months ago by jdemeyer

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

comment:18 in reply to: ↑ 15 ; follow-up: Changed 20 months ago by 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?

comment:19 in reply to: ↑ 18 Changed 20 months ago by tscrim

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.

comment:20 Changed 20 months ago by SimonKing

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.

comment:21 Changed 20 months ago by tscrim

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

comment:22 Changed 20 months ago by tscrim

I would try it with just -t.

comment:23 Changed 20 months ago by SimonKing

The undue doctest slowness did not go away with sage -t. Here is what I got:

[p_group_cohomology-3.0.1] Using --optional=ccache,database_gap,gap_packages,gfortran,meataxe,mpir,python2,sage
[p_group_cohomology-3.0.1] Doctesting 12 files.
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/cochain.pyx
[p_group_cohomology-3.0.1]     [1459 tests, 293.21 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/barcode.py
[p_group_cohomology-3.0.1]     [143 tests, 11.61 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/modular_cohomology.pyx
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/modular_cohomology.pyx", line 132, in pGroupCohomology.modular_cohomology._IdGroup
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     _IdGroup(G2,D,H)
[p_group_cohomology-3.0.1] Test ran for 38.77 s
[p_group_cohomology-3.0.1]     [452 tests, 247.64 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/resolution_bindings.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/cohomology.pyx
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 10525, in pGroupCohomology.cohomology.COHO._poincare_series_old_implementation
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     for n in range(1,268):               #long time
[p_group_cohomology-3.0.1]         H = CohomologyRing(64, n)
[p_group_cohomology-3.0.1]         H.make()
[p_group_cohomology-3.0.1]         if H.poincare_series() != H._poincare_series_old_implementation(): # indirect doctest
[p_group_cohomology-3.0.1]             print(n)
[p_group_cohomology-3.0.1] Test ran for 59.41 s
[p_group_cohomology-3.0.1]     [1381 tests, 302.76 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/cochain.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/factory.py
[p_group_cohomology-3.0.1]     [216 tests, 43.12 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/auxiliaries.py
[p_group_cohomology-3.0.1]     [69 tests, 1.87 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/__init__.py
[p_group_cohomology-3.0.1] **********************************************************************
[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 561.11 s
[p_group_cohomology-3.0.1]     [349 tests, 644.05 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/dickson.pyx
[p_group_cohomology-3.0.1]     [28 tests, 1.57 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/resolution.pyx
[p_group_cohomology-3.0.1]     [882 tests, 10.83 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 38.6 pyxsources/pGroupCohomology/resolution.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] ----------------------------------------------------------------------
[p_group_cohomology-3.0.1] All tests passed!
[p_group_cohomology-3.0.1] ----------------------------------------------------------------------
[p_group_cohomology-3.0.1] Total time for all tests: 1562.4 seconds
[p_group_cohomology-3.0.1]     cpu time: 709.4 seconds
[p_group_cohomology-3.0.1]     cumulative wall time: 1556.7 seconds

So, it is not unlikely that some stale data has accumulated in Singular, which thus became slow. The next thing to try: Let CohomologyRing.doctest_setup() restart Singular. In that way, the test would be closer to what one does in an interactive session.

comment:24 Changed 20 months ago by SimonKing

The situation has slightly improved: When restarting Singular more often, I get

[p_group_cohomology-3.0.1] Using --optional=ccache,database_gap,gap_packages,gfortran,meataxe,mpir,python2,sage
[p_group_cohomology-3.0.1] Doctesting 12 files.
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/cochain.pyx
[p_group_cohomology-3.0.1]     [1459 tests, 289.01 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/barcode.py
[p_group_cohomology-3.0.1]     [143 tests, 12.56 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/modular_cohomology.pyx
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/modular_cohomology.pyx", line 132, in pGroupCohomology.modular_cohomology._IdGroup
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     _IdGroup(G2,D,H)
[p_group_cohomology-3.0.1] Test ran for 39.07 s
[p_group_cohomology-3.0.1]     [452 tests, 245.99 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/resolution_bindings.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/cohomology.pyx
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 10525, in pGroupCohomology.cohomology.COHO._poincare_series_old_implementation
[p_group_cohomology-3.0.1] Warning, slow doctest:
[p_group_cohomology-3.0.1]     for n in range(1,268):               #long time
[p_group_cohomology-3.0.1]         H = CohomologyRing(64, n)
[p_group_cohomology-3.0.1]         H.make()
[p_group_cohomology-3.0.1]         if H.poincare_series() != H._poincare_series_old_implementation(): # indirect doctest
[p_group_cohomology-3.0.1]             print(n)
[p_group_cohomology-3.0.1] Test ran for 53.76 s
[p_group_cohomology-3.0.1]     [1381 tests, 314.09 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/cochain.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/factory.py
[p_group_cohomology-3.0.1]     [216 tests, 43.32 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/auxiliaries.py
[p_group_cohomology-3.0.1]     [69 tests, 2.24 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/__init__.py
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 984, in pGroupCohomology
[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 36.48 s
[p_group_cohomology-3.0.1] **********************************************************************
[p_group_cohomology-3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 2386, 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 561.76 s
[p_group_cohomology-3.0.1]     [348 tests, 644.03 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/dickson.pyx
[p_group_cohomology-3.0.1]     [28 tests, 1.56 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/resolution.pyx
[p_group_cohomology-3.0.1]     [882 tests, 11.85 s]
[p_group_cohomology-3.0.1] sage -t --long --warn-long 35.6 pyxsources/pGroupCohomology/resolution.pxd
[p_group_cohomology-3.0.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.0.1] ----------------------------------------------------------------------
[p_group_cohomology-3.0.1] All tests passed!
[p_group_cohomology-3.0.1] ----------------------------------------------------------------------
[p_group_cohomology-3.0.1] Total time for all tests: 1592.2 seconds
[p_group_cohomology-3.0.1]     cpu time: 729.7 seconds
[p_group_cohomology-3.0.1]     cumulative wall time: 1564.6 seconds
[p_group_cohomology-3.0.1] 
[p_group_cohomology-3.0.1] real	26m36.275s
[p_group_cohomology-3.0.1] user	13m11.855s
[p_group_cohomology-3.0.1] sys	2m25.020s

Edit

I originally thought I'd seen an improvement, but it's as bad as before.

So, what do you recommend to do?

Last edited 20 months ago by SimonKing (previous) (diff)

comment:25 Changed 20 months ago by git

  • Commit changed from 0c40f3efc06cbc01f946cb6704090a704cf64a02 to f7c8acf5d0f259411628c288b74b63149ba8c605

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

83c8ac2Upgrade to Singular 4.1.1p3
f10f27cMerge branch 't/25993/ticket/25993' into t/26001/upgrade_p_group_cohomology_to_version_3_0_1
f7c8acfp_group_cohomology-3.0.1: Some upstream fixes

comment:26 Changed 20 months ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Work around Singular bugs and slowness deleted

I have pushed my recent changes. The tests should pass.

Problems:

  1. Some tests are MUCH slower than they should be. In an interactive session, they work a lot faster. I do not have the faintest idea what is causing the problem - if I understand correctly, it has not been the case with previous Singular versions.
  2. While the documentation builds fine in a separate directory, it totally fails to correctly build during package installation.

Do you have an idea how to fix the above? If not, then I hope it doesn't prevent the package from being reviewed. In that way, it would at least be possible to upgrade to Singular-4.1.1.p3.

comment:27 Changed 19 months ago by SimonKing

Ping.

Meanwhile I consider to release version 3.1 of the cohomology package. News and changes:

  • I found that in an important example Singular has not been able to compute the Hilbert Poincaré series, because of int overflow (upstream didn't answer my question whether it is possible to make Singular use bigint). I changed my programs so that my own implementation is used when the int overflow occurs.
  • My own implementation of Hilbert Poincaré series computation will be improved and extended in the next version. It incorporates techniques that I learnt from an article of Anna Bigatti, whereas the old implementation followed the less efficient algorithm presented in Greuel's and Pfister's book.
  • The reason why I want to improve the computation of Hilbert Poincaré series: I want to verify a conjecture of Dave Benson on the so-called filter degree type of modular group cohomology rings. So far, I thought that the computation of the filter degree type involves very expensive computations, but recently I realised that all what needs to be done is computed a couple of Hilbert Poincaré series. And this will be in v3.1, too.

QUESTION

What shall happen with this ticket? The options I see are:

  1. Open a new ticket for p_group_cohomology-3.1 and close this one as wontfix.
  2. Open a new ticket for v3.1, but still wait for a review here.
  3. Change the topic of this ticket from "Upgrade to v3.0.1" to "Upgrade to v3.1".

Note that I still have no clue concerning slow doctests (that aren't slow interactively and weren't slow in previous package versions) and the docbuild failure (that only occurs while installing the package, but not when building the docs in a separate step).

comment:28 Changed 19 months ago by tscrim

I would say go with option 3.

comment:29 Changed 18 months ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Upgrade p_group_cohomology to version 3.0.1 to Upgrade p_group_cohomology to version 3.1
  • Work issues set to Provide v3.1 instad of 3.0.1

comment:30 Changed 18 months ago by SimonKing

This ticket is a dependency of #25993, but currently I have #25993 in my branch for this ticket. Is that a problem? I think a logical resolution would be to merge both into the same Sage release.

comment:31 Changed 18 months ago by tscrim

There are two options. The first is what you propose: merge both simultaneously. The second is to provide a patch file on #25993, and here, when you upgrade, you remove the patch file because of the upgrade. While the latter seems more complicated, it is in line with how we would handle the same situation for other packages (but usually with a less direct link to upstream). So I would say go with the first and make sure both are merged simultaneously (we might have to ask Volker about how to do this so his release scripts would handle it gracefully).

comment:32 follow-up: Changed 16 months ago by dimpase

See #26901 for updating this package for new GAP 4.10. While it's desirable to update to libgap as well, the more logical procedure would be to get rid of pexpect GAP in core GAP-related functionality of Sage, in particular in permutation groups. Otherwise one has extra hassle interfacing libgap and gap, see e.g. #26889 (which is by the way ready for review).

comment:33 in reply to: ↑ 32 ; follow-up: Changed 16 months ago by SimonKing

Replying to dimpase:

See #26901 for updating this package for new GAP 4.10. While it's desirable to update to libgap as well, the more logical procedure would be to get rid of pexpect GAP in core GAP-related functionality of Sage, in particular in permutation groups. Otherwise one has extra hassle interfacing libgap and gap, see e.g. #26889 (which is by the way ready for review).

I am not sure if I understand what you are saying.

Are you saying that #26901 updates GAP but not libgap, and I shouldn't do a transition of my package to libgap till Sage got rid of pexpect GAP? That wouldn't be an option for me.

By the way, inside of the cohomology package I am not using Sage's groups at all. So, for me it simply doesn't matter whether Sage uses GAP or libgap for permutation groups. In the old version of my package, I directly use pexpect-GAP to do group computations, and in the upcoming version, I will directly use libgap for the same purpose. So, no Sage-groups are involved, if that's what you meant in your comment.

So, my plan is: Do the transition to libgap now. Reading the "work issues" of #22626, I will probably be done before #22626 is ready for review. And then, if issues arise, I can upgrade p_group_cohomology again (which will likely be trivial compared with the transition from pexpect GAP to libgap).

comment:34 in reply to: ↑ 33 ; follow-up: Changed 16 months ago by dimpase

Replying to SimonKing:

Replying to dimpase:

See #26901 for updating this package for new GAP 4.10. While it's desirable to update to libgap as well, the more logical procedure would be to get rid of pexpect GAP in core GAP-related functionality of Sage, in particular in permutation groups. Otherwise one has extra hassle interfacing libgap and gap, see e.g. #26889 (which is by the way ready for review).

I am not sure if I understand what you are saying.

Are you saying that #26901 updates GAP but not libgap, and I shouldn't do a transition of my package to libgap till Sage got rid of pexpect GAP? That wouldn't be an option for me.

Certainly, #26901 updates GAP and libGAP, moreover it gets rid of Sage's libGAP in favour of GAP's native libgap, which has finally arrived.

By the way, inside of the cohomology package I am not using Sage's groups at all. So, for me it simply doesn't matter whether Sage uses GAP or libgap for permutation groups. In the old version of my package, I directly use pexpect-GAP to do group computations, and in the upcoming version, I will directly use libgap for the same purpose. So, no Sage-groups are involved, if that's what you meant in your comment.

So, my plan is: Do the transition to libgap now.

OK, in this case it sounds like a good plan.

Reading the "work issues" of #22626,

they haven't been updated for 5 weeks, it's now quite close to be finished in fact. Erik has been working on it basically full-time for a couple of weeks.

comment:35 in reply to: ↑ 34 Changed 16 months ago by SimonKing

  • Dependencies changed from #24735 to #24735 #22626

Replying to dimpase:

Reading the "work issues" of #22626,

they haven't been updated for 5 weeks, it's now quite close to be finished in fact. Erik has been working on it basically full-time for a couple of weeks.

I see. In that case, I think it will be safe to use #22626 as a dependency and try the switch from pexpect to libgap with the new version.

comment:36 Changed 16 months ago by SimonKing

I just tried to build the current development version of my package (manually, i.e., using python setup.py build). I get this warning:

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/Cython/Compiler/Main.py:367: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /home/king/Projekte/coho/coho-devel/src/pGroupCohomology/cochain.pxd

What to do?

comment:37 follow-up: Changed 16 months ago by SimonKing

OK, I guess it is

cythonize(ext_mods, compiler_directives={'embedsignature': True,
                                                       'language_level': 2})

comment:38 in reply to: ↑ 37 Changed 15 months ago by jdemeyer

Replying to SimonKing:

OK, I guess it is

cythonize(ext_mods, compiler_directives={'embedsignature': True,
                                                       'language_level': 2})

Yes, adding the language_level should fix that deprecation warning.

comment:39 Changed 15 months ago by SimonKing

AFAIC, the only remaining critical problem of the cohomology package after the switch to libgap is in the computation of barcodes. I am currently rewriting that function from scratch.

After that, I hope that the other doctest errors will only be changed logging or changed sorting. So, hopefully it is ready for review before the end of the year.

comment:40 Changed 15 months ago by SimonKing

  • Dependencies changed from #24735 #22626 to #24735 #22626 #26389

comment:41 Changed 15 months ago by SimonKing

I am trying to create a branch with all the dependencies for this ticket. My idea was to checkout the current develop branch and then to do git trac pull from the dependencies. Result:

  • git trac pull 24735 says there is nothing to do, so, I guess it is merged already.
  • git trac pull 22626 fails:
    king@klap:~/Sage/git/sage$ git trac checkout 22626
    Loading ticket #22626...
    Checking out Trac #22626 remote branch b446ebb496d45c4408aa949f98f855f962d9388a -> local branch t/22626/b446ebb496d45c4408aa949f98f855f962d9388a...
    Traceback (most recent call last):
      File "/home/king/.local/bin/git-trac", line 18, in <module>
        cmdline.launch()
      File "/home/king/Sage/git/git-trac-command/git_trac/cmdline.py", line 220, in launch
        app.checkout(args.ticket_or_branch, args.branch_name)
      File "/home/king/Sage/git/git-trac-command/git_trac/app.py", line 118, in checkout
        self._checkout_ticket(int(ticket_or_branch), branch_name)
      File "/home/king/Sage/git/git-trac-command/git_trac/app.py", line 146, in _checkout_ticket
        self.repo.checkout_new_branch(ticket.branch, branch)
      File "/home/king/Sage/git/git-trac-command/git_trac/git_repository.py", line 136, in checkout_new_branch
        self.git.fetch('trac', remote)
      File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 341, in meth
        return self.execute(git_cmd, *args, **kwds)
      File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 328, in execute
        popen_stderr=subprocess.PIPE)
      File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 263, in _run
        raise GitError(result)
    git_trac.git_error.GitError: git returned with non-zero exit code (1) when executing "git fetch trac b446ebb496d45c4408aa949f98f855f962d9388a"
    

So, what can I do?

comment:42 Changed 15 months ago by SimonKing

Strangely enough, it works now.

comment:43 follow-up: Changed 15 months ago by SimonKing

If I understand correctly, the package dependency database_gap can now be removed.

comment:44 in reply to: ↑ 43 Changed 15 months ago by dimpase

Replying to SimonKing:

If I understand correctly, the package dependency database_gap can now be removed.

of course,it must - there is no more such package after #22626

comment:45 Changed 15 months ago by SimonKing

  • Description modified (diff)
  • Work issues Provide v3.1 instad of 3.0.1 deleted

comment:46 Changed 15 months ago by SimonKing

  • Branch u/SimonKing/upgrade_p_group_cohomology_to_version_3_0_1 deleted
  • Commit f7c8acf5d0f259411628c288b74b63149ba8c605 deleted

comment:47 Changed 15 months ago by SimonKing

  • Branch set to u/SimonKing/upgrade_p_group_cohomology_to_version_3_1

comment:48 Changed 15 months ago by SimonKing

  • Commit set to 53f3e3f4e0272be687514a00b3836851a2624ccf
  • Status changed from needs_work to needs_review

I think the work is done. For me, the new version 3.1 of p_group_cohomology works with what currently is in develop, i.e., with Singular-4.1.1p2.p0 and GAP-4.10.0.

Note that the total computation time for all groups of order 64 is now only about 6:15min walltime.

sage: from pGroupCohomology.factory import unit_test_64
sage: unit_test_64()
...
#267: Walltime   6:12.67 min
      CPU-time   5:46.87 min
      Singular   0:42.96 min

([], [372.67809295654297, 346.87493, 42.97])

In the first package version, it used to be 45min or so.

Changes wrt. v3.0:

  • Hilbert series computation by using a new implementation in SageMath.
  • Vastly improved computation of filter degree type (now relying on Hilbert series).
  • Use libgap instead of the GAP pexpect interface.
  • Sub-package upgrade: modres-1.1
  • New routine to compute filter regular parameters in small degrees by enumeration.
  • Cope with some changes in Singular.

Concerning the last point: It has often been the case that changes in Singular or in the Sage library forced me to create a new version of the cohomology package. While this is still mentioned in SPKG.txt, I removed such details from the changelog that is part of the package documentation.

Plan for version 4.0: Replace the Singular pexpect interface with libsingular. It will be a lot more work than the change from gap to libgap, but I am sure that it will bring even more speed-up and stability, since the singular interface is used much more intensely than the gap interface.

Anyway: I think it can now be reviewed!


New commits:

68ac3e1Merge commit 'b446ebb496d45c4408aa949f98f855f962d9388a' of git://trac.sagemath.org/sage into m/26001/dependencies_for_p_group_cohomology-3.1
82f9454Merge branch 'develop' into m/26001/dependencies_for_p_group_cohomology-3.1
6ecff0bUpgrade to p_group_cohomology-3.0.1
53f3e3fUpgrade to p_group_cohomology-3.1

comment:49 Changed 15 months ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Upstream changes in documentation

Sorry, I found that I should do some editorial changes.

While I am at it, I now think that I should try to remove some of the very long tests. Indeed, some of theses tests only demonstrate how one should not proceed, and some of these tests appear twice in two different locations.

So, wait a few more hours before you review ;-)...

Last edited 15 months ago by SimonKing (previous) (diff)

comment:50 Changed 15 months ago by git

  • Commit changed from 53f3e3f4e0272be687514a00b3836851a2624ccf to 318f0ad695ebd0a5cbd0d5164dc1fe9cc3005e50

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

318f0adUpdate p_group_cohomology-3.1 checksum

comment:51 Changed 15 months ago by SimonKing

  • Status changed from needs_work to needs_review

Now it's done. Copied from sage -f -c p_group_cohomology:

[p_group_cohomology-3.1] PASS: mnttest.sh
[p_group_cohomology-3.1] ============================================================================
[p_group_cohomology-3.1] Testsuite summary for modular_resolution 1.1
[p_group_cohomology-3.1] ============================================================================
[p_group_cohomology-3.1] # TOTAL: 1
[p_group_cohomology-3.1] # PASS:  1
[p_group_cohomology-3.1] # SKIP:  0
[p_group_cohomology-3.1] # XFAIL: 0
[p_group_cohomology-3.1] # FAIL:  0
[p_group_cohomology-3.1] # XPASS: 0
[p_group_cohomology-3.1] # ERROR: 0
[p_group_cohomology-3.1] ============================================================================
...
[p_group_cohomology-3.1] Running doctests with ID 2018-12-29-16-14-41-55dab330.
[p_group_cohomology-3.1] Git branch: t/26001/upgrade_p_group_cohomology_to_version_3_1
[p_group_cohomology-3.1] Using --optional=ccache,database_gap,dochtml,frobby,gap_packages,gdb,gfortran,meataxe,memlimit,mpir,python2,sage
[p_group_cohomology-3.1] Sorting sources by runtime so that slower doctests are run first....
[p_group_cohomology-3.1] Doctesting 12 files using 3 threads.
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/cochain.pxd
[p_group_cohomology-3.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/cohomology.pyx
[p_group_cohomology-3.1] **********************************************************************
[p_group_cohomology-3.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 5303, in pGroupCohomology.cohomology.COHO.essential_ideal
[p_group_cohomology-3.1] Warning, slow doctest:
[p_group_cohomology-3.1]     I = H.essential_ideal()    #long time
[p_group_cohomology-3.1] Test ran for 57.93 s
[p_group_cohomology-3.1]     [1372 tests, 313.75 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/modular_cohomology.pyx
[p_group_cohomology-3.1]     [459 tests, 271.15 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/cochain.pyx
[p_group_cohomology-3.1]     [1460 tests, 345.80 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/factory.py
[p_group_cohomology-3.1] **********************************************************************
[p_group_cohomology-3.1] File "pyxsources/pGroupCohomology/factory.py", line 116, in pGroupCohomology.factory.unit_test_64
[p_group_cohomology-3.1] Warning, slow doctest:
[p_group_cohomology-3.1]     L,t = unit_test_64(from_scratch=False)    # long time
[p_group_cohomology-3.1] Test ran for 37.09 s
[p_group_cohomology-3.1]     [216 tests, 46.28 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/barcode.py
[p_group_cohomology-3.1]     [143 tests, 12.35 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/dickson.pyx
[p_group_cohomology-3.1]     [28 tests, 1.52 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/auxiliaries.py
[p_group_cohomology-3.1]     [39 tests, 0.86 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/resolution.pxd
[p_group_cohomology-3.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/resolution_bindings.pxd
[p_group_cohomology-3.1]     [0 tests, 0.00 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/__init__.py
[p_group_cohomology-3.1]     [343 tests, 104.25 s]
[p_group_cohomology-3.1] sage -t --long --warn-long 30.9 pyxsources/pGroupCohomology/resolution.pyx
[p_group_cohomology-3.1]     [882 tests, 11.17 s]
[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: 384.9 seconds
[p_group_cohomology-3.1]     cpu time: 779.8 seconds
[p_group_cohomology-3.1]     cumulative wall time: 1107.1 seconds
[p_group_cohomology-3.1] 
[p_group_cohomology-3.1] real	6m27.444s
[p_group_cohomology-3.1] user	15m15.893s
[p_group_cohomology-3.1] sys	2m21.782s
[p_group_cohomology-3.1] Successfully installed p_group_cohomology-3.1
[p_group_cohomology-3.1] Deleting temporary build directory
[p_group_cohomology-3.1] /home/king/Sage/git/sage/local/var/tmp/sage/build/p_group_cohomology-3.1
[p_group_cohomology-3.1] Finished installing p_group_cohomology-3.1.spkg

So, there are only two tests for which a "long time" warning arises.

  • One long test demonstrates a result on "essential classes" and show-cases capabilities of the method essential_ideal(). This test actually used to appear in the docs twice, but I have removed one copy of that test.
  • The other long test is a "quick" version of unit_test_64. The full test would take more than 6 minutes and would compute the cohomology rings of all 268 groups of order 64 from scratch, and compare with the stored results. The "quick" version merely tests that the 268 cohomology rings can all be unpickled. So, it tests the integrity of the data base that is shipped with the package.

I prefer to keep both long tests.

Anyway. Since sage.tests.modular_group_cohomology and the package's test suite pass, I think it is now in the shape to be reviewed.

comment:52 Changed 15 months ago by tscrim

  • Milestone changed from sage-8.4 to sage-8.6
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review
  • Work issues Upstream changes in documentation deleted

LGTM.

comment:53 follow-up: Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:54 in reply to: ↑ 53 Changed 15 months ago by SimonKing

Replying to vbraun:

Merge conflict

Please be more specific when you claim a merge conflict.

All what this ticket does is to touch files in build/pkgs/p_group_cohomology and src/sage/tests/modular_group_cohomology/, and I guess as the author of that package I would know if there had been a recent conflicting change in these files.

comment:55 Changed 15 months ago by SimonKing

  • Dependencies changed from #24735 #22626 #26389 to #24735 #22626 #26389 #26856

It seems that the merge conflict with #26856. Volker, next time please just tell it.

comment:56 Changed 15 months ago by SimonKing

Gosh, what is wrong now?

king@klap:~/Sage/git/sage$ git trac checkout 26856
Loading ticket #26856...
Checking out Trac #26856 remote branch 8bf1044efda750d8a767d88aa6a2afda69edadbe -> local branch t/26856/8bf1044efda750d8a767d88aa6a2afda69edadbe...
Traceback (most recent call last):
  File "/home/king/.local/bin/git-trac", line 18, in <module>
    cmdline.launch()
  File "/home/king/Sage/git/git-trac-command/git_trac/cmdline.py", line 220, in launch
    app.checkout(args.ticket_or_branch, args.branch_name)
  File "/home/king/Sage/git/git-trac-command/git_trac/app.py", line 118, in checkout
    self._checkout_ticket(int(ticket_or_branch), branch_name)
  File "/home/king/Sage/git/git-trac-command/git_trac/app.py", line 146, in _checkout_ticket
    self.repo.checkout_new_branch(ticket.branch, branch)
  File "/home/king/Sage/git/git-trac-command/git_trac/git_repository.py", line 136, in checkout_new_branch
    self.git.fetch('trac', remote)
  File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 341, in meth
    return self.execute(git_cmd, *args, **kwds)
  File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 328, in execute
    popen_stderr=subprocess.PIPE)
  File "/home/king/Sage/git/git-trac-command/git_trac/git_interface.py", line 263, in _run
    raise GitError(result)
git_trac.git_error.GitError: git returned with non-zero exit code (1) when executing "git fetch trac 8bf1044efda750d8a767d88aa6a2afda69edadbe"

comment:57 follow-up: Changed 15 months ago by dimpase

How about just using plain git?

(git trac is really more of a release manager tool nowadays, using it on closed tickets is iffy...)

comment:58 in reply to: ↑ 57 ; follow-up: Changed 15 months ago by SimonKing

Replying to dimpase:

How about just using plain git?

(git trac is really more of a release manager tool nowadays, using it on closed tickets is iffy...)

How?

comment:59 in reply to: ↑ 58 Changed 15 months ago by dimpase

Replying to SimonKing:

Replying to dimpase:

How about just using plain git?

(git trac is really more of a release manager tool nowadays, using it on closed tickets is iffy...)

How?

assuming trac is the tag of the correct remote, (i.e. the output of git remote -v contains the line trac git@trac.sagemath.org:sage.git (fetch) ), do

git fetch trac public/ticket-26856
git checkout -b foo_whatever FETCH_HEAD

checks out the branch of #26856 (which is still there, just not reflected in the ticket's form any more; you can see the name in #26856, comment 137) and tags it foo_whatever. If you don't need to give it a name, then the last line could be just

git checkout FETCH_HEAD

comment:60 Changed 15 months ago by vbraun

IMHO doing anything with closed tickets is iffy; If the branch was changed in the meantime (because the author deleted it, or reused the branch name for another ticket) then its easy to get into a situaiton where you either cannot download it (only named branches can be downloaded, not sha1s) or where you are merging in the wrong code.

comment:61 Changed 15 months ago by git

  • Commit changed from 318f0ad695ebd0a5cbd0d5164dc1fe9cc3005e50 to 2c99aca999e2932ed58ec6219cde8e47f67bf90e

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

2c99acaMerge branch 'develop' into t/26001/upgrade_p_group_cohomology_to_version_3_1

comment:62 follow-up: Changed 15 months ago by SimonKing

  • Status changed from needs_work to needs_review

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.

comment:63 Changed 15 months ago by dimpase

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:

2c99acaMerge branch 'develop' into t/26001/upgrade_p_group_cohomology_to_version_3_1

comment:64 in reply to: ↑ 62 Changed 15 months ago by dimpase

Replying to SimonKing:

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.

comment:65 Changed 15 months ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Cope with a downstream change

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.

comment:66 Changed 15 months ago by SimonKing

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

comment:67 follow-up: Changed 15 months ago by dimpase

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

Last edited 15 months ago by dimpase (previous) (diff)

comment:68 in reply to: ↑ 67 Changed 15 months ago by SimonKing

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.

comment:69 Changed 15 months ago by dimpase

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)

comment:70 Changed 15 months ago by git

  • Commit changed from 2c99aca999e2932ed58ec6219cde8e47f67bf90e to 076cbf20c8ac18a1ef3d3d9c364f8dcc299f7ae2

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

076cbf2p_group_cohomology: Avoid conflict with a new global variable name in Gap

comment:71 Changed 15 months ago by SimonKing

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

comment:72 Changed 15 months ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Cope with a downstream change deleted

comment:73 follow-up: Changed 15 months ago by dimpase

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Dima Pasechnik
  • Status changed from needs_review to positive_review

Happy New Year, Simon :-)

comment:74 in reply to: ↑ 73 Changed 15 months ago by SimonKing

Replying to dimpase:

Happy New Year, Simon :-)

To you as well, Dima!

comment:75 Changed 15 months ago by jdemeyer

  • Cc vbraun added
  • Priority changed from major to blocker

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

comment:76 Changed 15 months ago by vbraun

  • Branch changed from u/SimonKing/upgrade_p_group_cohomology_to_version_3_1 to 076cbf20c8ac18a1ef3d3d9c364f8dcc299f7ae2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.