Opened 11 years ago

Closed 11 years ago

# Problems with S-class groups of number fields

Reported by: Owned by: fwclarke davidloeffler major sage-4.7.2 number fields S-class groups jdemeyer, cremona, rlm sage-4.7.2.alpha1 Francis Clarke John Cremona N/A

### Description

There are some serious problems at present with the code for S-class groups. They only emerge when the class groups is non-cyclic. For example,

```sage: K.<a> = QuadraticField(-105)
sage: C = K.class_group(); C
Class group of order 8 with structure C2 x C2 x C2 of Number Field in a with defining polynomial x^2 + 105
sage: S = (K.ideal(11, a + 7),)
sage: K.S_class_group(S)
Traceback (most recent call last):
...
IndexError: Argument length (= 3) must be 2.
```

This problem arises when the class group and the S-class group have differing numbers of generators. It arises essentially because generators of S-class groups are created as `FractionalIdealClass` elements rather than `SFractionalIdealClass` elements.

But there is a more serious problem. The Pari data for the S-class group which we failed to construct above can be obtained as

```sage: SC_data = K._S_class_group_and_units(S)[1]; SC_data
[(Fractional ideal (10, a + 5), 2, 10), (Fractional ideal (6, a + 3), 2, 6)]
```

so that if

```sage: P, Q = [u[0] for u in SC_data]
```

the S-classes of the ideals `P` and `Q` (each of order 2) generate the S-class group. However,

```sage: P._S_ideal_class_log(S)
[0, 0]
```

which cannot be correct.

### Changed 11 years ago by fwclarke

apply after patches from #11234

### comment:1 Changed 11 years ago by fwclarke

• Authors set to Francis Clarke
• Status changed from new to needs_review

The patch rectifies the defects. It also incorporates enhancements for both class groups and S-class groups.

In particular:

The computation of S-class groups is made faster when the ideals in S are all principal.

The computation of ideal_class_logs is speeded up by setting a pari flag.

The code for S-class groups is made more compatible with that for class groups.

### comment:2 follow-up: ↓ 3 Changed 11 years ago by cremona

Looks good to me, and applies fine to 4.7.1.alpha1 and all tests pass.

I've added rlm to the CC list since I think he implemented this originally, and hope he'll take a look too.

### comment:3 in reply to: ↑ 2 ; follow-up: ↓ 6 Changed 11 years ago by mstreng

Looks good to me, and applies fine to 4.7.1.alpha1 and all tests pass.

I've added rlm to the CC list since I think he implemented this originally, and hope he'll take a look too.

As Robert doesn't seem to reply, do you want to give this a positive review, or have someone else review it too?

### comment:4 Changed 11 years ago by cremona

• Reviewers set to John Cremona
• Status changed from needs_review to positive_review

Let's give it a positive review, since I clearly thought it deserved that 5 weeks ago! Marco, you can add your name as reviewer if appropriate.

### comment:5 Changed 11 years ago by jdemeyer

• Milestone changed from sage-4.7.1 to sage-4.7.2

### comment:6 in reply to: ↑ 3 Changed 11 years ago by rlm

Looks good to me, and applies fine to 4.7.1.alpha1 and all tests pass.

I've added rlm to the CC list since I think he implemented this originally, and hope he'll take a look too.

As Robert doesn't seem to reply, do you want to give this a positive review, or have someone else review it too?

Sorry for the delay, but I can add that the changes here look good to me too.

### comment:7 Changed 11 years ago by jdemeyer

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