Opened 13 years ago

Closed 13 years ago

#3719 closed defect (fixed)

[with spkg, patch, positive review] bug in group cohomology

Reported by: wdj Owned by: tbd
Priority: major Milestone: sage-3.1.2
Component: algebra Keywords:
Cc: AlexGhitza Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

As reported by Ursula Whitcher

sage: G = SymmetricGroup(4)
sage: G.cohomology(1,2)

yields an error. The problem is a bug in the current version of the GAP package HAP, version 1.8.5. The latest version 1.8.7 but there the bug still exists

gap> G := SymmetricGroup(4);
Sym( [ 1 .. 4 ] )
gap> GroupCohomology(G,1); ## an improvement over 1.8.5
[  ]
gap> GroupCohomology(G,1,2);
List Element: <position> must be a positive integer (not a integer) at
if IsInt( C!.fpIntHom[n] )  then
...

Attachments (2)

10128.patch (2.1 KB) - added by wdj 13 years ago.
docstring addition patch based on 3.1.alpha0
10129.patch (4.0 KB) - added by wdj 13 years ago.
based on 3.1.alpha0 and probably the previous patch

Download all attachments as: .zip

Change History (14)

comment:1 Changed 13 years ago by mabshoff

  • Milestone set to sage-3.1.1

comment:3 Changed 13 years ago by wdj

Sorry, meant to add that this passes sage -testall on amd64 hardy heron and includes hap1.8.8 which fixes the problem reported above.

sage: gap.eval('LoadPackage("hap")')
'true'
sage: G = SymmetricGroup(4)
sage: G.cohomology(1,2)
Multiplicative Abelian Group isomorphic to C2

comment:4 Changed 13 years ago by mabshoff

  • Summary changed from bug in group cohomology to [with spkg, needs review] bug in group cohomology

David,

please also add an optional doctest that verifies that the issue has been fixed.

Cheers,

Michael

comment:5 follow-up: Changed 13 years ago by wdj

Michael: You mean, supply a patch to permgroup.py with this addition to the docstring to the group_cohomology method for a new trac ticket?

comment:6 in reply to: ↑ 5 Changed 13 years ago by mabshoff

Replying to wdj:

Michael: You mean, supply a patch to permgroup.py with this addition to the docstring to the group_cohomology method for a new trac ticket?

Yes, it should be an optional doctest. I am somewhat concerned that the author of the GAP package just rereleased the package with the bug fix instead of incrementing the release number. Since various packaging efforts are under way I could imagine something like this not getting fixed upstream in other distributions, so the doctest when run with optional flags would show that the problem remains unfixed.

It is still my plan to used optional testing in the future at least for the build on sage.math per default.

Cheers,

Michael

Changed 13 years ago by wdj

docstring addition patch based on 3.1.alpha0

comment:7 Changed 13 years ago by wdj

Okay, I just attached the patch you requested to this ticket. (I wasn't sure if it needed a new ticket or not.) It passes sage -t but it dawned on me afterwards that sage -t would not test for optional docstring additions. Anyway, hope this is what you were looking for.

BTW, I am one of the webmasters for GAP (hence involved wityh package updates) and you can be sure that hap 1.8.8 will definitely get applied upstream, probably in the next week or so.

comment:8 Changed 13 years ago by was

Okay, I just attached the patch you requested to this ticket. (I wasn't sure

if it needed a new ticket or not.) It passes sage -t but it dawned on

me afterwards that sage -t would not test for optional docstring additions.

Use

  sage -t --optional 

Changed 13 years ago by wdj

based on 3.1.alpha0 and probably the previous patch

comment:9 Changed 13 years ago by wdj

Okay, the attached passes sage -t --optional I had to make some changes to the code. For some reason, it seems gap packages were not loading for me properly (eg, gap.eval('LoadPackage?("hap")') gave a traceback the first time but loaded it the second time...). That was fixed with some try/except trickery which seems harmless even if packages do load as they should. Also, GAP's RequirePackage? is actually being deprecated, so I replaced them with the preferred LoadPackage?. Finally, Molien series do not require an optional package, so I removed some comments in the docstring for that method. Hope this is okay now.

comment:10 Changed 13 years ago by AlexGhitza

  • Cc AlexGhitza added

comment:11 Changed 13 years ago by AlexGhitza

  • Summary changed from [with spkg, needs review] bug in group cohomology to [with spkg and two patches, positive review] bug in group cohomology

Looks good. The new spkg fixes the problem that was reported, and the few things that broke in the upgrade are fixed by the two patches.

comment:12 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from [with spkg and two patches, positive review] bug in group cohomology to [with spkg, patch, positive review] bug in group cohomology

Merged both patches in Sage 3.1.2.alpha1.

David,

I added an hg repo to the spkg (but kept the p6 patchlevel). Please base future spkgs on the spkg

http://sage.math.washington.edu/home/mabshoff/release-cycles-3.1.2/alpha1/gap_packages-4.4.10_6.spkg

Cheers,

Michael

Note: See TracTickets for help on using tickets.