Problems with S-class groups of number fields

[sagetrac-fwclarke]

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.

CC: @jdemeyer @JohnCremona @rlmill

Component: number fields

Keywords: S-class groups

Author: Francis Clarke

Reviewer: John Cremona

Merged: sage-4.7.2.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/11304

[sagetrac-fwclarke]

Attachment: trac_11304_S_class_groups.patch.gz

apply after patches from #11234

[sagetrac-fwclarke]

Author: Francis Clarke

[sagetrac-fwclarke]

comment:1

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.

[JohnCremona]

comment:2

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.

[mstreng]

comment:3

Replying to @JohnCremona:

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?

[JohnCremona]

comment:4

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.

[JohnCremona]

Reviewer: John Cremona

[rlmill]

comment:6

Replying to @mstreng:

Replying to @JohnCremona:

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.

[jdemeyer]

Merged: sage-4.7.2.alpha1