#6105: [with patch, needs work] Additions and changes in Group Algebras
-------------------------+--------------------------------------------------
Reporter: jlefebvre | Owner: tbd
Type: enhancement | Status: new
Priority: minor | Milestone: sage-4.0.2
Component: algebra | Keywords: Group Algebra
Reviewer: | Author:
Merged: |
-------------------------+--------------------------------------------------
Comment(by davidloeffler):
I haven't tried installing this, but here are some comments based on
browsing the code.
It looks like this is excellent functionality, but there are some things
I'm not very happy with.
(1) Brutally converting all finite groups into permutation groups will
mean we can't generally create an element of the group algebra from an
element of the group, which means that most of the basic arithmetic
operations that I originally wrote the class for won't work any more.
Please don't do this; rather, add more functionality to the basic Group
class, or (if absolutely necessary) make GroupAlgebra store both the
original group and its permutation form (and the mapping between them).
(2) Many of these docstrings are somewhat confusing; I can't for the life
of me work out what the docstring for "good_for" is trying to say -- some
of it seems to be about the function at hand, some about another unnamed
function, and nowhere is it defined what the function actually does.
(3) Lots of these functions need more testing. There are some obvious
typos, e.g. in is_noetherian:
{{{
if self.group().is_polycyclic and self.base_ring().is_field():
}}}
should read
{{{
if self.group().is_polycyclic() and self.base_ring().is_field():
}}}
When there are lots of different cases handled in a function, it's good to
have a test for each case -- the aim is that "sage -testall" should test
every single line of code in the Sage library, and much of this code
simply won't get hit. Then typos like this come out in the wash.
(4) It would be good if the GroupAlgebra class could be incorporated into
the reference manual (by adding a line to
{{{sage/doc/en/reference/algebras.rst}}}). To do this will require lots of
formatting changes to the docstrings (e.g. example blocks must look like
{{{
EXAMPLE::
sage: some code
}}}
with two colons and a blank line). This is somewhat orthogonal to your
patch and is actually my fault -- when I wrote the group algebra class, I
forgot to add it to the reference manual, so it never got updated when we
changed to a new and better reference manual compilation system -- but it
seems a shame to make the problem worse by adding many new functions with
new docstrings that are not ReSTified.
Much of this is cosmetic, but (1) is a big issue -- I don't think we can
consider merging this into Sage if it's going to break creating elements
of group algebras for most of our finite group classes. So I'm changing
this back to "needs work"; sorry.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6105#comment:5>
Sage <http://sagemath.org/>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
