Opened 14 years ago

Closed 11 years ago

# [superseded by #6449] rewrite abelian groups

Reported by: Owned by: was joyner major sage-duplicate/invalid/wontfix group theory editor_roed N/A

### Description

```
On Jan 19, 2008 8:00 AM, John Cremona <john.cremona@gmail.com> 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/abelian_groups.py, 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.

William
```

### comment:1 Changed 14 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 14 years ago by was

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

### Changed 14 years ago by was

this is part 2. This is also not ready.

### comment:2 Changed 14 years ago by was

I just found this terrifying line in `abelian_group.py`:

```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/abelian_group.py 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)
704
705     def __cmp__(self, right):

/Users/was/sage2/local/lib/python2.5/site-packages/sage/groups/abelian_gps/abelian_group.py 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/gap.py 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/expect.py 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/gap.py in _eval_line(self, line, allow_use_file, wait_for_prompt)
508                         return ''
509                 else:
--> 510                     raise RuntimeError, message
511
512         except KeyboardInterrupt:

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

executing gensH:=[aa];
```

### comment:3 Changed 14 years ago by mabshoff

#517 is in the same ball park.

Cheers,

Michael

### 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 11 years ago by mpatel

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