Opened 13 years ago

Closed 12 years ago

Last modified 3 weeks ago

#9332 closed enhancement (fixed)

S_class_group() should return a group

Reported by: stankewicz Owned by: David Loeffler
Priority: major Milestone: sage-4.6.2
Component: number fields Keywords:
Cc: Alyson Deines, John Cremona, Jeremy West Merged in: sage-4.6.2.alpha1
Authors: Jim Stankewicz, Erin Beyerstedt, Anna Haensch, Robert Miller, David Loeffler Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (1)

trac_9332.patch (17.4 KB) - added by Robert Miller 12 years ago.
rebased on sage-4.6.1.alpha2

Download all attachments as: .zip

Change History (29)

comment:1 Changed 13 years ago by stankewicz

Status: newneeds_review

comment:2 Changed 13 years ago by Robert Miller

Status: needs_reviewneeds_work

I don't think there's a point to adding a TestSuite? doctest that doesn't work. We can always add that in once #7945 is fixed. You also need to make good on your IOU. I would also suggest a shorter repr string-- you don't want it line-wrapping on the terminal unnecessarily.

comment:3 Changed 13 years ago by Anna Haensch

Authors: stankewiczJim Stankewicz, Erin Beyerstedt, Anna Haensch

There are still some doctest failures in number_field.py, but we've added documentation and improved the way S_class_groups print. Joint work with Erin Beyerstedt, Robert Miller, H.

comment:4 Changed 13 years ago by John Cremona

Good work so far!

I think the repr string for a (s-)class group should not mention its order or structure at all. The user can always ask for that.

I'm not sure about the S() function returning None for a straight class group. I would be happy with it returning [] when S is None.

In the number field file, there is no need to add the parameter None for straght class groups, since that is the default value; so take those out.

I will now try the patches out...

comment:5 Changed 13 years ago by John Cremona

(After testing) When you have simplified the string output of an S-class group you can adjust the doctest outputs in number_field.py. (Don't worry about those Minkowski warnings -- they will disappear when the doctest passes!)

comment:6 Changed 13 years ago by ebeyerstedt

Description: modified (diff)

comment:7 Changed 13 years ago by John Cremona

For me the 3rd patch does not apply on top of the first two. It also looks strange -- does it mix tabs and spaces? (You should use only spaces, some editors put tabs in).

comment:8 Changed 13 years ago by stankewicz

Cc: adeines John Cremona Jeremy West added

The newest patch is something that I've been working on for the last several days, but I'm making no progress. There's a phantom attribute error which comes up under the following code:

sage: K.<a> = QuadraticField(-14)                             
sage: K.testSclassgroup(tuple([K.ideal(2,a)]))                
Class group of order 2 with structure C2 of Number Field in a with defining polynomial x^2 + 14
sage: K.testSclassgroup(tuple([K.ideal(2,a)]))(K.ideal(3,a+1))
ERROR: An unexpected error occurred ...

Perhaps there's something stupid I'm missing and someone else can point it out. Otherwise, I'm going to work on something else for a while and come back to this with a fresh head.

comment:9 Changed 13 years ago by David Loeffler

The AttributeError? comes up because attributes whose names start with an underscore are private, and ones with two underscores are "very private" -- so private that they aren't even accessible to subclasses. So when code in FractionalIdealClass sets __ideal, code in the subclass SFractionalIdealClass. In fact it is still accessible but under a mangled name: see http://docs.python.org/tutorial/classes.html#private-variables.

But there is another, deeper, bug which will be uncovered when you fix this. After my changes at #9244, a FractionalIdealClass stores both a representing ideal of that class, accessed via self.ideal(), and an expression for self in terms of the generators of its parent class group, accessed via self.list(). The way you've set this up, elements of an S-class group use the same data structure, but the data which list() uses is set completely wrongly: it's set to the exponents of the given ideal in terms of the generators of the class group, but it should be in terms of the generators of the S-class group. What you need to do is to store somewhere the expressions for the generators of the S-class group in terms of the generators of the class group (i.e. the matrix of the quotient map) and then multiply the output of _ideal_class_log by this.

This is the sort of stuff that #6449 was intended to solve: quotients of abelian groups should be automatically handled by general code, rather than having to do it by hand in every specific example.

comment:10 in reply to:  9 Changed 13 years ago by John Cremona

Replying to davidloeffler:

The AttributeError? comes up because attributes whose names start with an underscore are private, and ones with two underscores are "very private" -- so private that they aren't even accessible to subclasses. So when code in FractionalIdealClass sets __ideal, code in the subclass SFractionalIdealClass. In fact it is still accessible but under a mangled name: see http://docs.python.org/tutorial/classes.html#private-variables.

But there is another, deeper, bug which will be uncovered when you fix this. After my changes at #9244, a FractionalIdealClass stores both a representing ideal of that class, accessed via self.ideal(), and an expression for self in terms of the generators of its parent class group, accessed via self.list(). The way you've set this up, elements of an S-class group use the same data structure, but the data which list() uses is set completely wrongly: it's set to the exponents of the given ideal in terms of the generators of the class group, but it should be in terms of the generators of the S-class group. What you need to do is to store somewhere the expressions for the generators of the S-class group in terms of the generators of the class group (i.e. the matrix of the quotient map) and then multiply the output of _ideal_class_log by this.

This is the sort of stuff that #6449 was intended to solve: quotients of abelian groups should be automatically handled by general code, rather than having to do it by hand in every specific example.

David, you should have been at Sage Days!

comment:11 Changed 13 years ago by David Loeffler

Here's a patch that does the equivalent of _ideal_class_log for S-class groups; you might find it useful.

comment:12 Changed 13 years ago by stankewicz

Here's patch that finally makes the S-class-group work as a group, on top of Loeffler's patch(in this ticket). Soon to come will be a single patch that gets it all done, and actually has acceptable documentation.

comment:13 Changed 13 years ago by Anna Haensch

Status: needs_workneeds_review

comment:14 Changed 13 years ago by Anna Haensch

Description: modified (diff)

Correction: trac_9332_v3 should be applied only on trac_9244_ver4. Sorry for the confusion, trac_9332_v3 includes changes made in previous patches, so there's no need to apply them. Cheers.

comment:15 Changed 13 years ago by Robert Miller

Let's clean up this ticket. Shall I delete the patches other than trac_9332_v3 and trac_9244_ver4?

comment:16 Changed 13 years ago by David Loeffler

All patches here can be deleted except trac_9332_v3. (trac_9244_ver4 is from another ticket. )

comment:17 Changed 13 years ago by Robert Miller

Description: modified (diff)

Cleaned up the other one as well.

comment:18 Changed 12 years ago by John Cremona

Status: needs_reviewneeds_work

Can we get this ticket moving again? The one and only patch still here does not apply cleanly to 4.6.rc0, so the first step is to fix that.

comment:19 Changed 12 years ago by Robert Miller

Authors: Jim Stankewicz, Erin Beyerstedt, Anna HaenschJim Stankewicz, Erin Beyerstedt, Anna Haensch, Robert Miller, David Loeffler
Description: modified (diff)

I'm uploading a rebased (on sage-4.6) patch which modifies printing in class fields to not print the structure when size is 1, fixes some caching bugs, updates several doctests, etc...

However, this happens, and I'm not sure whether this is okay or not:

sage -t sage/rings/number_field/number_field_ideal.py
**********************************************************************
File "/home/rlmill/sage-4.6/devel/sage-main/sage/rings/number_field/number_field
_ideal.py", line 1017:
    sage: I._S_ideal_class_log([])
Expected:
    [3]
Got:
    [1]
**********************************************************************

comment:20 Changed 12 years ago by John Cremona

Status: needs_workneeds_review

I applied the patch (no problems) to 4.6.1.alpha2. The thing you observed does not happen:

sage: K.<a> = QuadraticField(-14) 
sage: S = K.primes_above(2) 
sage: S
[Fractional ideal (2, a)]
sage: I = K.ideal(3, a-1)
sage: I._S_ideal_class_log(S) 
[1]
sage: I._S_ideal_class_log([]) 
[1]

I am now testing...

comment:21 Changed 12 years ago by John Cremona

Reviewers: John Cremona

Two trivial doctest failures

sage -t -long sage/rings/polynomial/polynomial_quotient_ring.py
**********************************************************************
File "/home/jec/sage-4.6.1.alpha2/devel/sage-main/sage/rings/polynomial/polynomial_quotient_ring.py", line 737:
    sage: K.class_group()
Expected:
    Class group of order 1 with structure  of Number Field in a with defining polynomial x^2 + 3
Got:
    Class group of order 1 of Number Field in a with defining polynomial x^2 + 3

and

sage -t -long sage/rings/number_field/number_field_ideal.py
**********************************************************************
File "/home/jec/sage-4.6.1.alpha2/devel/sage-main/sage/rings/number_field/number_field_ideal.py", line 1051:
    sage: I._S_ideal_class_log([])
Expected:
    [3]
Got:
    [1]

where in both cases the "Expected " does not look right anyway!

I am leaving this as needs review since I have not looked closely again at the code, but I can assert that apart from the above two things all (long) tests pass with 4.6.1.alpha2.

comment:22 Changed 12 years ago by Robert Miller

I also believe the actual output is correct. Evidence:

sage: K.<a> = QuadraticField(-14)
sage: S = K.primes_above(2)
sage: I = K.ideal(3,a+2)
sage: I
Fractional ideal (3, a + 2)
sage: I._ideal_class_log()
[1]
sage: I._S_ideal_class_log([])
[1]

I am uploading a fresh patch, which will fix all the above issues.

comment:23 Changed 12 years ago by John Cremona

Status: needs_reviewpositive_review

New patch is fine!

comment:24 Changed 12 years ago by Jeroen Demeyer

Milestone: sage-4.6.2

comment:25 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

Nice patch but you should change the commit message :-)

Changed 12 years ago by Robert Miller

Attachment: trac_9332.patch added

rebased on sage-4.6.1.alpha2

comment:26 Changed 12 years ago by Robert Miller

Status: needs_workpositive_review

comment:27 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.alpha1
Resolution: fixed
Status: positive_reviewclosed

comment:28 Changed 3 weeks ago by Frédéric Chapoton

Cc: Alyson Deines added; adeines removed
Note: See TracTickets for help on using tickets.