Opened 10 years ago

Closed 8 years ago

#6670 closed enhancement (fixed)

Port group algebras to the current coercion system

Reported by: mraum Owned by: mraum
Priority: minor Milestone: sage-4.7.2
Component: algebra Keywords:
Cc: Merged in: sage-4.7.2.alpha2
Authors: Martin Raum, John Palmieri Reviewers: John Palmieri, Martin Raum
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mraum)

This upgrades the group algebras to the current coercion system and fixes some issues of multiplication of group algebras A and B, satisfying A == B but not admitting coercion of elements. This depends on #6669, which concerns homomorphisms from matrix group to other objects.

Apply:

  1. trac_6670-jhp.patch
  2. trac-6670-functors.patch

Attachments (9)

trac-6670-group_algebra.patch (19.8 KB) - added by mraum 10 years ago.
trac-6670-group_algebra-2.patch (21.8 KB) - added by mraum 10 years ago.
This is a rebase to 4.1.2
trac-6670-group_algebra-3.patch (21.3 KB) - added by mraum 10 years ago.
This is a rebase to 4.3alpha0 .
trac-6670-group_algebra-4.patch (21.5 KB) - added by mraum 10 years ago.
This applies cleanly to a fresh 4.3.1
trac-6670-group_algebra-5.patch (27.9 KB) - added by davidloeffler 9 years ago.
Apply only this -- rebased against 4.5.alpha1 and docstrings ReSTified
trac-6670-group_algebra-6.patch (24.9 KB) - added by mraum 9 years ago.
trac-6670-group_algebra-7.patch (4.0 KB) - added by mraum 9 years ago.
trac_6670-jhp.patch (32.0 KB) - added by jhpalmieri 8 years ago.
trac-6670-functors.patch (2.2 KB) - added by mraum 8 years ago.

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by mraum

Changed 10 years ago by mraum

This is a rebase to 4.1.2

comment:1 Changed 10 years ago by mraum

  • Status changed from needs_work to needs_review
  • Summary changed from [with patch, needs work] Port group algebras to the current coercion system to [with patch, needs review] Port group algebras to the current coercion system

A patch for #6669 has been uploaded. This patch depends on it.

comment:2 Changed 10 years ago by AlexGhitza

  • Authors set to Martin Raum
  • Summary changed from [with patch, needs review] Port group algebras to the current coercion system to Port group algebras to the current coercion system

comment:3 Changed 10 years ago by robertwb

  • Status changed from needs_review to positive_review

There's a lot of new code here, but it all looks good, and the doctests are fine too.

Given the amount of category code that went into 4.3, we should make sure all tests pass when applied against that as well. (I tested against 4.2.)

comment:4 Changed 10 years ago by mhansen

  • Report Upstream set to N/A
  • Status changed from positive_review to needs_work

Needs some work/rebasing for 4.3

Changed 10 years ago by mraum

This is a rebase to 4.3alpha0 .

comment:5 Changed 10 years ago by mraum

  • Status changed from needs_work to needs_review

comment:6 Changed 10 years ago by robertwb

  • Status changed from needs_review to needs_work
	sage -t  devel/sage-main/sage/modular/modsym/space.py # Segfault
	sage -t  devel/sage-main/sage/algebras/group_algebra.py # 5 doctests failed
	sage -t  devel/sage-main/sage/interfaces/sage0.py # 1 doctests failed

Changed 10 years ago by mraum

This applies cleanly to a fresh 4.3.1

comment:7 Changed 10 years ago by mraum

  • Status changed from needs_work to needs_review

comment:8 Changed 10 years ago by roed

I'm hoping to take a look at this, but if someone else has time soon and wants to beat me to it, go for it.

comment:9 Changed 10 years ago by pierre

  • Status changed from needs_review to needs_work

-- This patch does not apply on sage 4.3.3. Mercurial error message :

applying trac-6670-group_algebra-4.patch patching file sage/algebras/group_algebra.py Hunk #1 FAILED at 27 Hunk #7 succeeded at 358 with fuzz 2 (offset 9 lines). Hunk #8 FAILED at 361 2 out of 10 hunks FAILED -- saving rejects to file sage/algebras/group_algebra.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac-6670-group_algebra-4.patch

-- You may want to have a look at the following related bug :

sage: G= SymmetricGroup?(5); x, y= G.gens() sage: A= GroupAlgebra?(G) sage: A( A(x) )

...fails. This bug may or may not be automatically fixed by your patch.

-- The docstring on line 367 of group_algebra.py mentions GroupAlgebra?.call(), even though this method has been suppressed.

comment:10 Changed 9 years ago by davidloeffler

  • Work issues set to unpickling problems

I spent quite a while rebasing this to 4.5.alpha1. I will upload the rebased patch; but I was disappointed to find that there is still a serious issue that needs to be addressed.

The problem is that unpickling elements of group algebras created using the old code fails; you can't replace a class name with a function name and expect it to unpickle seamlessly. It ends up expecting sage.algebras.group_algebra.GroupAlgebraElement to be a class which can just be filled in with the pickled __dict__ values, not a callable. This is what I get if I pickle a group algebra element without the patch, apply the patch and try to unpickle:

sage: load("/home/masiao/gpalg.sobj")
---------------------------------------------------------------------------
UnpicklingError                           Traceback (most recent call last)

/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/algebras/<ipython console> in <module>()

/storage/masiao/sage-4.5.alpha1/local/lib/python2.6/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.load (sage/structure/sage_object.c:7577)()

/storage/masiao/sage-4.5.alpha1/local/lib/python2.6/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.loads (sage/structure/sage_object.c:9175)()

UnpicklingError: NEWOBJ class argument isn't a type object

Changed 9 years ago by davidloeffler

Apply only this -- rebased against 4.5.alpha1 and docstrings ReSTified

Changed 9 years ago by mraum

Changed 9 years ago by mraum

comment:11 Changed 9 years ago by mraum

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues unpickling problems deleted

I added a completely new file containing the new implementation. The old one is deprecated now, but it still exists and pickling works. William suggested to remove the old implementation in 5.0, which I will open a ticket for as soon as this ticket is merged.

comment:12 Changed 8 years ago by chapoton

For the bot:

Apply trac-6670-group_algebra-6.patch, trac-6670-group_algebra-7.patch

comment:13 Changed 8 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

Some minor comments:

  • The first line of the new file is redundant: "Group algebra of a group". Maybe it should just be "Group algebras"?
  • The new file should be added to the reference manual, by adding an appropriate line to sage/doc/en/reference/algebras.rst.

More interesting:

  • Can you implement coercion from R[H] to R[G] if H is a subgroup of G, or more generally from R[H] to S[G] if there is a coercion from H to G and from R to S? Then coercing from the base ring R to R[G] would just be the special case where R=S and H is the trivial group.

A more important issue: I'm not sure that I agree with the implementation. I would have it inherit from CombinatorialFreeModule, and then unique representation is built in nicely so you don't have to cache the results as you do now. You can also implement the Hopf algebra structure on the group algebra pretty easily. For reference, you should look at the files

  • sage/categories/examples/hopf_algebras_with_basis.py for a simple (not very full-featured) implementation of group algebras.
  • sage/algebras/steenrod/steenrod_algebra.py for the implementation of the Steenrod algebra, including all of its Hopf algebra structure, inheriting from CombinatorialFreeModule. This has a lot of things you don't need, but if you want to base the implementation on the first file, or if you want to modify the print representation of elements (which I recommend), you might be able to use this one to help fill in some details.

comment:14 Changed 8 years ago by jhpalmieri

  • Authors changed from Martin Raum to Martin Raum, John Palmieri
  • Description modified (diff)
  • Status changed from needs_work to needs_review

Here's a new version, which makes the changes I suggested, and also adds some documentation at the top of the file. I think I preserved all of the tests from the previous version, so we're not losing any functionality. There are some new things, like an antipode and a comultiplication for elements.

Changed 8 years ago by jhpalmieri

comment:15 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:16 Changed 8 years ago by mraum

I'm afraid the new version is quite similar to the original (though much better), so that I wouldn't be a legitimate reviewer. On the other hand, since the changes that you introduced are located in some function that you added I could review this part, as soon as I am back from holidays and you could review the other part. Do you think this is a legitimate solution?

One thing: The functor needs to be modified, using the new apply methods, that are available thanks to Simon's work. I will do this, if you don't do it first.

comment:17 Changed 8 years ago by mraum

I think it is important to note that on #11318 it is suggested unifying GroupAlgebra?(G, A) and G.algebra(A). But I think we should still get this patch into Sage to have at least some more modern construction until we finally have the implementation anticipated in #11318.

Changed 8 years ago by mraum

comment:18 Changed 8 years ago by mraum

  • Description modified (diff)

I attached a patch that changes the way functors are called.

Also I reviewed the parts that you implemented. That is, the transition to CombinatorialFreeModule?. I would give this a positive review. So if you agree with the procedure that I described above and are fine with the rest of the code you can give this a positive review as a whole.

comment:19 Changed 8 years ago by jhpalmieri

  • Milestone changed from sage-4.7.1 to sage-4.7.2
  • Reviewers set to John Palmieri, Martin Raum
  • Status changed from needs_review to positive_review

Looks good to me. The point about #11318 is a good one, although I don't like the way elements of G.algebra(R) are printed, compared to elements of GroupAlgebra(G,R).

comment:20 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.