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: |
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)
Change History (14)
comment:1 Changed 13 years ago by
- Milestone set to sage-3.1.1
comment:2 Changed 13 years ago by
comment:3 Changed 13 years ago by
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
- 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: ↓ 6 Changed 13 years ago by
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
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
comment:7 Changed 13 years ago by
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
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
comment:9 Changed 13 years ago by
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
- Cc AlexGhitza added
comment:11 Changed 13 years ago by
- 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
- 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
Cheers,
Michael
I posted a new gap_packages archive to http://sage.math.washington.edu/home/wdj/patches/gap_packages-4.4.10_6.spkg