Opened 4 years ago
Closed 3 years ago
#26001 closed defect (fixed)
Upgrade p_group_cohomology to version 3.1
Reported by:  SimonKing  Owned by:  

Priority:  blocker  Milestone:  sage8.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, GitHub, GitLab)  Commit:  076cbf20c8ac18a1ef3d3d9c364f8dcc299f7ae2 
Dependencies:  #24735 #22626 #26389 #26856  Stopgaps: 
Description (last modified by )
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_cohomology3.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_cohomology3.1.
Change History (76)
comment:1 Changed 4 years ago by
 Dependencies set to #24735
comment:2 followup: ↓ 3 Changed 4 years ago by
 Cc jdemeyer added
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 4 years ago by
comment:4 in reply to: ↑ 3 Changed 4 years ago by
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 4 years ago by
 Cc tscrim added
comment:6 Changed 4 years ago by
 Branch set to u/SimonKing/upgrade_p_group_cohomology_to_version_3_0_1
comment:7 Changed 4 years ago by
 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 builtin 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 Singular4.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 4 years ago by
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 4 years ago by
With Singular4.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 Singular4.1.1.p3. So, I have to return to using my Hilbert driven computations.
comment:10 Changed 4 years ago by
 Status changed from needs_review to needs_work
 Work issues set to Work around Singular bugs and slowness
comment:11 Changed 4 years ago by
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 4 years ago by
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 Singular4.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 4 years ago by
Concerning the timings, I am not sure what is happening. As part of the test suite, I get
[p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 5178, in pGroupCohomology.cohomology.COHO.essential_ideal [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] I = H.essential_ideal() #long time [p_group_cohomology3.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 4 years ago by
Even more extreme is that one:
[p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 2391, in pGroupCohomology [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] H.make(14) [p_group_cohomology3.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 followups: ↓ 16 ↓ 18 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 ; followup: ↓ 19 Changed 4 years ago by
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 4 years ago by
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 sagedevel thread.
comment:20 Changed 4 years ago by
From spgkcheck:
# 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 4 years ago by
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 4 years ago by
I would try it with just t
.
comment:23 Changed 4 years ago by
The undue doctest slowness did not go away with sage t
. Here is what I got:
[p_group_cohomology3.0.1] Using optional=ccache,database_gap,gap_packages,gfortran,meataxe,mpir,python2,sage [p_group_cohomology3.0.1] Doctesting 12 files. [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/cochain.pyx [p_group_cohomology3.0.1] [1459 tests, 293.21 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/barcode.py [p_group_cohomology3.0.1] [143 tests, 11.61 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/modular_cohomology.pyx [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/modular_cohomology.pyx", line 132, in pGroupCohomology.modular_cohomology._IdGroup [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] _IdGroup(G2,D,H) [p_group_cohomology3.0.1] Test ran for 38.77 s [p_group_cohomology3.0.1] [452 tests, 247.64 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/resolution_bindings.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/cohomology.pyx [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 10525, in pGroupCohomology.cohomology.COHO._poincare_series_old_implementation [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] for n in range(1,268): #long time [p_group_cohomology3.0.1] H = CohomologyRing(64, n) [p_group_cohomology3.0.1] H.make() [p_group_cohomology3.0.1] if H.poincare_series() != H._poincare_series_old_implementation(): # indirect doctest [p_group_cohomology3.0.1] print(n) [p_group_cohomology3.0.1] Test ran for 59.41 s [p_group_cohomology3.0.1] [1381 tests, 302.76 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/cochain.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/factory.py [p_group_cohomology3.0.1] [216 tests, 43.12 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/auxiliaries.py [p_group_cohomology3.0.1] [69 tests, 1.87 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/__init__.py [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 2391, in pGroupCohomology [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] H.make(14) [p_group_cohomology3.0.1] Test ran for 561.11 s [p_group_cohomology3.0.1] [349 tests, 644.05 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/dickson.pyx [p_group_cohomology3.0.1] [28 tests, 1.57 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/resolution.pyx [p_group_cohomology3.0.1] [882 tests, 10.83 s] [p_group_cohomology3.0.1] sage t long warnlong 38.6 pyxsources/pGroupCohomology/resolution.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1]  [p_group_cohomology3.0.1] All tests passed! [p_group_cohomology3.0.1]  [p_group_cohomology3.0.1] Total time for all tests: 1562.4 seconds [p_group_cohomology3.0.1] cpu time: 709.4 seconds [p_group_cohomology3.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 4 years ago by
The situation has slightly improved: When restarting Singular more often, I get
[p_group_cohomology3.0.1] Using optional=ccache,database_gap,gap_packages,gfortran,meataxe,mpir,python2,sage [p_group_cohomology3.0.1] Doctesting 12 files. [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/cochain.pyx [p_group_cohomology3.0.1] [1459 tests, 289.01 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/barcode.py [p_group_cohomology3.0.1] [143 tests, 12.56 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/modular_cohomology.pyx [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/modular_cohomology.pyx", line 132, in pGroupCohomology.modular_cohomology._IdGroup [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] _IdGroup(G2,D,H) [p_group_cohomology3.0.1] Test ran for 39.07 s [p_group_cohomology3.0.1] [452 tests, 245.99 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/resolution_bindings.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/cohomology.pyx [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 10525, in pGroupCohomology.cohomology.COHO._poincare_series_old_implementation [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] for n in range(1,268): #long time [p_group_cohomology3.0.1] H = CohomologyRing(64, n) [p_group_cohomology3.0.1] H.make() [p_group_cohomology3.0.1] if H.poincare_series() != H._poincare_series_old_implementation(): # indirect doctest [p_group_cohomology3.0.1] print(n) [p_group_cohomology3.0.1] Test ran for 53.76 s [p_group_cohomology3.0.1] [1381 tests, 314.09 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/cochain.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/factory.py [p_group_cohomology3.0.1] [216 tests, 43.32 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/auxiliaries.py [p_group_cohomology3.0.1] [69 tests, 2.24 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/__init__.py [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 984, in pGroupCohomology [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] I = H.essential_ideal() #long time [p_group_cohomology3.0.1] Test ran for 36.48 s [p_group_cohomology3.0.1] ********************************************************************** [p_group_cohomology3.0.1] File "pyxsources/pGroupCohomology/__init__.py", line 2386, in pGroupCohomology [p_group_cohomology3.0.1] Warning, slow doctest: [p_group_cohomology3.0.1] H.make(14) [p_group_cohomology3.0.1] Test ran for 561.76 s [p_group_cohomology3.0.1] [348 tests, 644.03 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/dickson.pyx [p_group_cohomology3.0.1] [28 tests, 1.56 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/resolution.pyx [p_group_cohomology3.0.1] [882 tests, 11.85 s] [p_group_cohomology3.0.1] sage t long warnlong 35.6 pyxsources/pGroupCohomology/resolution.pxd [p_group_cohomology3.0.1] [0 tests, 0.00 s] [p_group_cohomology3.0.1]  [p_group_cohomology3.0.1] All tests passed! [p_group_cohomology3.0.1]  [p_group_cohomology3.0.1] Total time for all tests: 1592.2 seconds [p_group_cohomology3.0.1] cpu time: 729.7 seconds [p_group_cohomology3.0.1] cumulative wall time: 1564.6 seconds [p_group_cohomology3.0.1] [p_group_cohomology3.0.1] real 26m36.275s [p_group_cohomology3.0.1] user 13m11.855s [p_group_cohomology3.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?
comment:25 Changed 4 years ago by
 Commit changed from 0c40f3efc06cbc01f946cb6704090a704cf64a02 to f7c8acf5d0f259411628c288b74b63149ba8c605
comment:26 Changed 4 years ago by
 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:
 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.
 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 Singular4.1.1.p3.
comment:27 Changed 4 years ago by
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 socalled 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:
 Open a new ticket for p_group_cohomology3.1 and close this one as wontfix.
 Open a new ticket for v3.1, but still wait for a review here.
 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 4 years ago by
I would say go with option 3.
comment:29 Changed 4 years ago by
 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 4 years ago by
comment:31 Changed 4 years ago by
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 followup: ↓ 33 Changed 3 years ago by
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 GAPrelated 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 ; followup: ↓ 34 Changed 3 years ago by
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 GAPrelated 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 pexpectGAP to do group computations, and in the upcoming version, I will directly use libgap for the same purpose. So, no Sagegroups 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 ; followup: ↓ 35 Changed 3 years ago by
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 GAPrelated 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 pexpectGAP to do group computations, and in the upcoming version, I will directly use libgap for the same purpose. So, no Sagegroups 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 fulltime for a couple of weeks.
comment:35 in reply to: ↑ 34 Changed 3 years ago by
 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 fulltime 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 3 years ago by
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/sitepackages/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/cohodevel/src/pGroupCohomology/cochain.pxd
What to do?
comment:37 followup: ↓ 38 Changed 3 years ago by
OK, I guess it is
cythonize(ext_mods, compiler_directives={'embedsignature': True, 'language_level': 2})
comment:38 in reply to: ↑ 37 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
 Dependencies changed from #24735 #22626 to #24735 #22626 #26389
comment:41 Changed 3 years ago by
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/gittrac", line 18, in <module> cmdline.launch() File "/home/king/Sage/git/gittraccommand/git_trac/cmdline.py", line 220, in launch app.checkout(args.ticket_or_branch, args.branch_name) File "/home/king/Sage/git/gittraccommand/git_trac/app.py", line 118, in checkout self._checkout_ticket(int(ticket_or_branch), branch_name) File "/home/king/Sage/git/gittraccommand/git_trac/app.py", line 146, in _checkout_ticket self.repo.checkout_new_branch(ticket.branch, branch) File "/home/king/Sage/git/gittraccommand/git_trac/git_repository.py", line 136, in checkout_new_branch self.git.fetch('trac', remote) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 341, in meth return self.execute(git_cmd, *args, **kwds) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 328, in execute popen_stderr=subprocess.PIPE) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 263, in _run raise GitError(result) git_trac.git_error.GitError: git returned with nonzero exit code (1) when executing "git fetch trac b446ebb496d45c4408aa949f98f855f962d9388a"
So, what can I do?
comment:42 Changed 3 years ago by
Strangely enough, it works now.
comment:43 followup: ↓ 44 Changed 3 years ago by
If I understand correctly, the package dependency database_gap
can now be removed.
comment:44 in reply to: ↑ 43 Changed 3 years ago by
comment:45 Changed 3 years ago by
 Description modified (diff)
 Work issues Provide v3.1 instad of 3.0.1 deleted
comment:46 Changed 3 years ago by
 Branch u/SimonKing/upgrade_p_group_cohomology_to_version_3_0_1 deleted
 Commit f7c8acf5d0f259411628c288b74b63149ba8c605 deleted
comment:47 Changed 3 years ago by
 Branch set to u/SimonKing/upgrade_p_group_cohomology_to_version_3_1
comment:48 Changed 3 years ago by
 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 Singular4.1.1p2.p0 and GAP4.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 CPUtime 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.
 Subpackage upgrade: modres1.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 speedup 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:
68ac3e1  Merge commit 'b446ebb496d45c4408aa949f98f855f962d9388a' of git://trac.sagemath.org/sage into m/26001/dependencies_for_p_group_cohomology3.1

82f9454  Merge branch 'develop' into m/26001/dependencies_for_p_group_cohomology3.1

6ecff0b  Upgrade to p_group_cohomology3.0.1

53f3e3f  Upgrade to p_group_cohomology3.1

comment:49 Changed 3 years ago by
 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 ;)
...
comment:50 Changed 3 years ago by
 Commit changed from 53f3e3f4e0272be687514a00b3836851a2624ccf to 318f0ad695ebd0a5cbd0d5164dc1fe9cc3005e50
Branch pushed to git repo; I updated commit sha1. New commits:
318f0ad  Update p_group_cohomology3.1 checksum

comment:51 Changed 3 years ago by
 Status changed from needs_work to needs_review
Now it's done. Copied from sage f c p_group_cohomology
:
[p_group_cohomology3.1] PASS: mnttest.sh [p_group_cohomology3.1] ============================================================================ [p_group_cohomology3.1] Testsuite summary for modular_resolution 1.1 [p_group_cohomology3.1] ============================================================================ [p_group_cohomology3.1] # TOTAL: 1 [p_group_cohomology3.1] # PASS: 1 [p_group_cohomology3.1] # SKIP: 0 [p_group_cohomology3.1] # XFAIL: 0 [p_group_cohomology3.1] # FAIL: 0 [p_group_cohomology3.1] # XPASS: 0 [p_group_cohomology3.1] # ERROR: 0 [p_group_cohomology3.1] ============================================================================ ... [p_group_cohomology3.1] Running doctests with ID 2018122916144155dab330. [p_group_cohomology3.1] Git branch: t/26001/upgrade_p_group_cohomology_to_version_3_1 [p_group_cohomology3.1] Using optional=ccache,database_gap,dochtml,frobby,gap_packages,gdb,gfortran,meataxe,memlimit,mpir,python2,sage [p_group_cohomology3.1] Sorting sources by runtime so that slower doctests are run first.... [p_group_cohomology3.1] Doctesting 12 files using 3 threads. [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/cochain.pxd [p_group_cohomology3.1] [0 tests, 0.00 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/cohomology.pyx [p_group_cohomology3.1] ********************************************************************** [p_group_cohomology3.1] File "pyxsources/pGroupCohomology/cohomology.pyx", line 5303, in pGroupCohomology.cohomology.COHO.essential_ideal [p_group_cohomology3.1] Warning, slow doctest: [p_group_cohomology3.1] I = H.essential_ideal() #long time [p_group_cohomology3.1] Test ran for 57.93 s [p_group_cohomology3.1] [1372 tests, 313.75 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/modular_cohomology.pyx [p_group_cohomology3.1] [459 tests, 271.15 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/cochain.pyx [p_group_cohomology3.1] [1460 tests, 345.80 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/factory.py [p_group_cohomology3.1] ********************************************************************** [p_group_cohomology3.1] File "pyxsources/pGroupCohomology/factory.py", line 116, in pGroupCohomology.factory.unit_test_64 [p_group_cohomology3.1] Warning, slow doctest: [p_group_cohomology3.1] L,t = unit_test_64(from_scratch=False) # long time [p_group_cohomology3.1] Test ran for 37.09 s [p_group_cohomology3.1] [216 tests, 46.28 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/barcode.py [p_group_cohomology3.1] [143 tests, 12.35 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/dickson.pyx [p_group_cohomology3.1] [28 tests, 1.52 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/auxiliaries.py [p_group_cohomology3.1] [39 tests, 0.86 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/resolution.pxd [p_group_cohomology3.1] [0 tests, 0.00 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/resolution_bindings.pxd [p_group_cohomology3.1] [0 tests, 0.00 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/__init__.py [p_group_cohomology3.1] [343 tests, 104.25 s] [p_group_cohomology3.1] sage t long warnlong 30.9 pyxsources/pGroupCohomology/resolution.pyx [p_group_cohomology3.1] [882 tests, 11.17 s] [p_group_cohomology3.1]  [p_group_cohomology3.1] All tests passed! [p_group_cohomology3.1]  [p_group_cohomology3.1] Total time for all tests: 384.9 seconds [p_group_cohomology3.1] cpu time: 779.8 seconds [p_group_cohomology3.1] cumulative wall time: 1107.1 seconds [p_group_cohomology3.1] [p_group_cohomology3.1] real 6m27.444s [p_group_cohomology3.1] user 15m15.893s [p_group_cohomology3.1] sys 2m21.782s [p_group_cohomology3.1] Successfully installed p_group_cohomology3.1 [p_group_cohomology3.1] Deleting temporary build directory [p_group_cohomology3.1] /home/king/Sage/git/sage/local/var/tmp/sage/build/p_group_cohomology3.1 [p_group_cohomology3.1] Finished installing p_group_cohomology3.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 showcases 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 3 years ago by
 Milestone changed from sage8.4 to sage8.6
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
 Work issues Upstream changes in documentation deleted
LGTM.
comment:53 followup: ↓ 54 Changed 3 years ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:54 in reply to: ↑ 53 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
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/gittrac", line 18, in <module> cmdline.launch() File "/home/king/Sage/git/gittraccommand/git_trac/cmdline.py", line 220, in launch app.checkout(args.ticket_or_branch, args.branch_name) File "/home/king/Sage/git/gittraccommand/git_trac/app.py", line 118, in checkout self._checkout_ticket(int(ticket_or_branch), branch_name) File "/home/king/Sage/git/gittraccommand/git_trac/app.py", line 146, in _checkout_ticket self.repo.checkout_new_branch(ticket.branch, branch) File "/home/king/Sage/git/gittraccommand/git_trac/git_repository.py", line 136, in checkout_new_branch self.git.fetch('trac', remote) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 341, in meth return self.execute(git_cmd, *args, **kwds) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 328, in execute popen_stderr=subprocess.PIPE) File "/home/king/Sage/git/gittraccommand/git_trac/git_interface.py", line 263, in _run raise GitError(result) git_trac.git_error.GitError: git returned with nonzero exit code (1) when executing "git fetch trac 8bf1044efda750d8a767d88aa6a2afda69edadbe"
comment:57 followup: ↓ 58 Changed 3 years ago by
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 ; followup: ↓ 59 Changed 3 years ago by
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 3 years ago by
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/ticket26856 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 3 years ago by
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 3 years ago by
 Commit changed from 318f0ad695ebd0a5cbd0d5164dc1fe9cc3005e50 to 2c99aca999e2932ed58ec6219cde8e47f67bf90e
Branch pushed to git repo; I updated commit sha1. New commits:
2c99aca  Merge branch 'develop' into t/26001/upgrade_p_group_cohomology_to_version_3_1

comment:62 followup: ↓ 64 Changed 3 years ago by
 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 3 years ago by
I hate to think what could happen if a ticket merged in a betarelease stage gets unmerged before the release, while its branch is gone... Anyway, I merely illustrated a basic twoliner 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

comment:64 in reply to: ↑ 62 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
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 followup: ↓ 68 Changed 3 years ago by
This is GAP (one of its packages, I guess) that is using GroupName
now:
$ ./sage gap ┌───────┐ GAP 4.10.0 of 01Nov2018 │ GAP │ https://www.gapsystem.org └───────┘ Architecture: x86_64pclinuxgnudefault64 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 namespaceswhereas 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...
comment:68 in reply to: ↑ 67 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
 Commit changed from 2c99aca999e2932ed58ec6219cde8e47f67bf90e to 076cbf20c8ac18a1ef3d3d9c364f8dcc299f7ae2
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

comment:71 Changed 3 years ago by
I have updated the tarball (same address as before). After the change, I get
[p_group_cohomology3.1]  [p_group_cohomology3.1] All tests passed! [p_group_cohomology3.1]  [p_group_cohomology3.1] Total time for all tests: 467.4 seconds [p_group_cohomology3.1] cpu time: 920.5 seconds [p_group_cohomology3.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 3 years ago by
 Status changed from needs_work to needs_review
 Work issues Cope with a downstream change deleted
comment:73 followup: ↓ 74 Changed 3 years ago by
 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 3 years ago by
comment:75 Changed 3 years ago by
 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 3 years ago by
 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
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.