Opened 13 years ago

Closed 10 years ago

#1849 closed defect (duplicate)

[superseded by #6449] rewrite abelian groups

Reported by: was Owned by: joyner
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: group theory Keywords: editor_roed
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


On Jan 19, 2008 8:00 AM, John Cremona <> wrote:
> Either this is a bug or I have been teaching students the wrong thing for years:
> (in 2.9.3)
> sage: A=AbelianGroup([7,8])
> sage: A.invariants()
>  [7,8]
> sage: A.elementary_divisors()
>  [7,8]
> There are two sets of invariants for a finite abelian group.  The
> "elementary divisors" should be a list of integers n_1,n_2,...,n_k
> with each dividing the next such that A is the direct sum of cyclic
> groups of order n_i.  In this case A is cyclic so the list should be
> just [56].  The other set is a list of prime powers and in this case
> is indeed [7,8] though we could argue about the ordering.
> Judging by the docstrings, the implemeter of these things in Sage does
> not agree with me, since the code seems to assume that the
> "invariants" are what I am calling the elem. divisors, and the elem.
> divisors are the prime-power factors of the invariants, sorted.  In
> which case the invariants is listed above are wrong!
> The Sage version of this is all explained in the documentation in file
> sage/groups/abelian_gps/, attributed to David Joyner
> .  Please will all the algebraists out there tell me whether tey agree
> with me or him!  I'm not in a position to consult textbooks right now.
>  The docs there refer to Henri Cohen's books so it is possible that
> there is a French/English split here BUT in any case the group I
> defined about is *cyclic* so one of the two outputs is wrong, even if
> we disagree which one!

The problem is that when David Joyner implemented this, doing 

sage: A=AbelianGroup([7,8])

would immediately change the invariants to be the elementary divisors. 
I.e., it was *impossible* 
to make an abelian group with specified generator orders -- the would
also change to be elementary divisors.   This was so
hugely painful to use that I fixed AbelianGroup so that one could create
a group with arbitrary generator orders.   Unfortunately, I evidently
didn't correctly update the elementary_divisors function.


Attachments (2)

trac-1849-abelian-group-rewrite-part1.patch (39.1 KB) - added by was 13 years ago.
this is part 1 -- doctests outside abelian groups directory will be broken.
abelian_groups_rewrite_part_2.patch (18.8 KB) - added by was 13 years ago.
this is part 2. This is also not ready.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 13 years ago by was

  • Summary changed from bug in elementary_divisors for abelian groups to [with patch (part 1 of 2); not ready for review] rewrite abelian groups

Changed 13 years ago by was

this is part 1 -- doctests outside abelian groups directory will be broken.

Changed 13 years ago by was

this is part 2. This is also not ready.

comment:2 Changed 13 years ago by was

I just found this terrifying line in

Hgensf = [x for x in Hgens if len(set(Ggens0).intersection(set(list(str(x)))))==0]

OUCH! This will completely break if one makes, e.g., abelian groups with generators that have more than one letter. For example, in current Sage

sage: G.<a,aa,aaa> = AbelianGroup(3,[5,0,0])
sage: G.subgroup([a])

Multiplicative Abelian Group isomorphic to C5, which is the subgroup of
Multiplicative Abelian Group isomorphic to C5 x Z x Z
generated by [a]
sage: G.subgroup([aa])
<type 'exceptions.RuntimeError'>          Traceback (most recent call last)

/Users/was/<ipython console> in <module>()

/Users/was/sage2/local/lib/python2.5/site-packages/sage/groups/abelian_gps/ in subgroup(self, gensH, names)
    701             if not(gensH[i] in G):
    702                 raise TypeError, "Subgroup generators must belong to the given group."
--> 703         return AbelianGroup_subgroup(self, gensH, names)
    705     def __cmp__(self, right):

/Users/was/sage2/local/lib/python2.5/site-packages/sage/groups/abelian_gps/ in __init__(self, ambient, gens, names)
    927                gap.eval(cmd)
    928         s2 = "gensH:=%s"%Hgensf #### remove from this the ones <--> 0 invar
--> 929         gap.eval(s2)
    930         s3 = 'H:=Subgroup(G,gensH)'
    931         gap.eval(s3)

/Users/was/sage2/local/lib/python2.5/site-packages/sage/interfaces/ in eval(self, x, newlines, strip)
    307         if len(x) == 0 or x[len(x) - 1] != ';':
    308             x += ';'
--> 309         s = Expect.eval(self, x)
    310         if newlines:
    311             return s

/Users/was/sage2/local/lib/python2.5/site-packages/sage/interfaces/ in eval(self, code, strip, **kwds)
    705         try:
    706             with gc_disabled():
--> 707                 return '\n'.join([self._eval_line(L, **kwds) for L in code.split('\n') if L != ''])
    708         except KeyboardInterrupt:
    709             # DO NOT CATCH KeyboardInterrupt, as it is being caught

/Users/was/sage2/local/lib/python2.5/site-packages/sage/interfaces/ in _eval_line(self, line, allow_use_file, wait_for_prompt)
    508                         return ''
    509                 else:
--> 510                     raise RuntimeError, message
    512         except KeyboardInterrupt:

<type 'exceptions.RuntimeError'>: Gap produced error output
Variable: 'aa' must have a value

   executing gensH:=[aa];

comment:3 Changed 13 years ago by mabshoff

#517 is in the same ball park.



comment:4 Changed 13 years ago by was

See also #3127 and #2272.

comment:5 Changed 13 years ago by craigcitro

  • Keywords editor_roed added

comment:6 Changed 11 years ago by davidloeffler

  • Report Upstream set to N/A
  • Status changed from needs_work to needs_review
  • Summary changed from [with patch (part 1 of 2); not ready for review] rewrite abelian groups to [superseded by #6449] rewrite abelian groups

I propose that we resolve this as "fixed" now #6449 has gone in.

comment:7 Changed 11 years ago by jdemeyer

  • Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix

comment:8 Changed 11 years ago by davidloeffler

  • Status changed from needs_review to positive_review

I'm deducing that you agree with my proposal to close this, so I'm setting it to "positive review". Release manager: please close as duplicate.

comment:9 Changed 10 years ago by mpatel

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.