-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port group algebras to the current coercion system #6670
Comments
Attachment: trac-6670-group_algebra.patch.gz |
Attachment: trac-6670-group_algebra-2.patch.gz This is a rebase to 4.1.2 |
comment:1
A patch for #6669 has been uploaded. This patch depends on it. |
Author: Martin Raum |
comment:3
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
Needs some work/rebasing for 4.3 |
Attachment: trac-6670-group_algebra-3.patch.gz This is a rebase to 4.3alpha0 . |
comment:6
|
Attachment: trac-6670-group_algebra-4.patch.gz This applies cleanly to a fresh 4.3.1 |
comment:8
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
-- This patch does not apply on sage 4.3.3. Mercurial error message : applying 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() ...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. |
Changed work issues from unpickling problems to none |
comment:12
For the bot: Apply trac-6670-group_algebra-6.patch, trac-6670-group_algebra-7.patch |
comment:13
Some minor comments:
More interesting:
A more important issue: I'm not sure that I agree with the implementation. I would have it inherit from
|
This comment has been minimized.
This comment has been minimized.
Changed author from Martin Raum to Martin Raum, John Palmieri |
comment:14
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. |
comment:15
Attachment: trac_6670-jhp.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:16
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. |
This comment has been minimized.
This comment has been minimized.
comment:18
Attachment: trac-6670-functors.patch.gz 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. |
Reviewer: John Palmieri, Martin Raum |
comment:19
Looks good to me. The point about #11318 is a good one, although I don't like the way elements of |
Merged: sage-4.7.2.alpha2 |
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:
Component: algebra
Author: Martin Raum, John Palmieri
Reviewer: John Palmieri, Martin Raum
Merged: sage-4.7.2.alpha2
Issue created by migration from https://trac.sagemath.org/ticket/6670
The text was updated successfully, but these errors were encountered: