Opened 11 years ago

Closed 11 years ago

#9244 closed defect (fixed)

Number field class group improvements

Reported by: davidloeffler Owned by: davidloeffler
Priority: major Milestone: sage-4.5.2
Component: number fields Keywords:
Cc: Merged in: sage-4.5.2.alpha0
Authors: David Loeffler Reviewers: Francis Clarke, John Cremona, Jim Stankewicz
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by cremona)

I was working on doctesting sage/rings/number_field/class_group.py, and I was unable to resist the temptation to rewrite it. (There were all sorts of failures and inconsistencies caused by the fact that ClassGroup derived from AbelianGroup, but FractionalIdealClass didn't derive from AbelianGroupElement.)

I have a patch for this, (which used to depend on #9242 but no longer does), which I will upload as soon as someone explains how to squash the _test_category() error.

Attachments (1)

trac_9244_ver4.patch (16.0 KB) - added by davidloeffler 11 years ago.
replaces all previous attempts

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 years ago by was

  • Milestone changed from sage-4.4.5 to sage-4.5

Milestone sage-4.4.5 deleted

comment:2 Changed 11 years ago by davidloeffler

  • Status changed from new to needs_review

Here's a patch. It doesn't depend on any other patches, contrary to what I wrote in the description.

comment:3 Changed 11 years ago by fwclarke

  • Status changed from needs_review to needs_work

These are definite improvements, well implemented, and doctests pass. Just one remark:

The eventuality envisaged in the comment at line 95 of the patched class_group.py (which should be referring to ideal classes rather ideals) ought to have its own doctest, such as:

sage: K.<a> = QuadraticField(-23)
sage: L.<b> = K.extension(x^2 - 2)
sage: CK = K.class_group()
sage: CL = L.class_group()
sage: [CL(I).list() for I in CK]
[[0], [2], [4]]

comment:4 Changed 11 years ago by davidloeffler

  • Status changed from needs_work to needs_review

Good point. Here's a new patch incorporating your suggestion. I also realised that one of the doctests seems to return different output in 4.4.4 than in 4.4.4.alpha0, so I've flagged it with #random.

comment:5 Changed 11 years ago by cremona

  • Authors set to David Loeffler
  • Description modified (diff)
  • Reviewers set to John Cremona, Jim Stankewicz

comment:6 Changed 11 years ago by cremona

  • Reviewers changed from John Cremona, Jim Stankewicz to Francis Clarke, John Cremona, Jim Stankewicz
  • Status changed from needs_review to needs_work

Jim and I have looked at this too (we are working on #9332) and think this is nearly good to go. (We could not see any random doctests!)

This is the only failure (we tested all of sage/rings/number_fields):

sage -t  "sage/rings/number_field/class_group.py"           
**********************************************************************
File "/home/john/sage-4.4.4/devel/sage-tests/sage/rings/number_field/class_group.py", line 144:
    sage: C.gen(0)
Expected:
    Fractional ideal class (130, 1/2*a + 137/2)
Got:
    Fractional ideal class (41, a - 10)
**********************************************************************
File "/home/john/sage-4.4.4/devel/sage-tests/sage/rings/number_field/class_group.py", line 146:
    sage: C.gen(1)
Expected:
    Fractional ideal class (7, a)
Got:
    Fractional ideal class (17, a)
**********************************************************************
1 items had failures:
   2 of   5 in __main__.example_7
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/john/.sage//tmp/.doctest_class_group.py
	 [3.1 s]

comment:7 Changed 11 years ago by stankewicz

  • Status changed from needs_work to needs_review

I've double-checked the failure. It's just a different choice of generators for the class group ( C250 x C2 for those keeping track)

sage: i = C(C.number_field().gen(),7)                
sage: j = C(C.number_field().gen(),17)               
sage: k = C((1/2)*C.number_field().gen() + 137/2,130)
sage: l = C(C.number_field().gen() - 10,41)          
sage: i.list()                                       
[0, 1]
sage: j.list()
[125, 1]
sage: k.list()
[1, 0]
sage: l.list()
[88, 1]
sage: l.order()
250
sage: (j*(l^125)).order()
2

comment:8 follow-up: Changed 11 years ago by davidloeffler

My bad, I forgot to qrefresh before exporting. Here's a new patch with the #random flags.

comment:9 in reply to: ↑ 8 Changed 11 years ago by fwclarke

  • Status changed from needs_review to positive_review

Replying to davidloeffler:

Here's a new patch with the #random flags.

Since this sorts out the failure that John and Jim found (but which I can't reproduce), it's a positive review.

comment:10 Changed 11 years ago by davidloeffler

  • Status changed from positive_review to needs_work

Wait a minute, this causes a doctest failure in William's Bordeaux 2008 examples, because calculating class groups with the optional argument proof=False isn't handled correctly: it still tries to bnfcertify. I will write an updated patch in a moment.

comment:11 follow-up: Changed 11 years ago by davidloeffler

  • Status changed from needs_work to needs_review

This patch handles the "proof" argument in a slightly different way in order to make sure that bnfcertify isn't called unnecessarily.

comment:12 Changed 11 years ago by davidloeffler

Sorry, I made a mess of uploading that. Patches trac_9244_new.patch and trac_9244_new.2.patch are identical, and both replace the previous trac_9244.patch.

comment:13 in reply to: ↑ 11 Changed 11 years ago by fwclarke

  • Status changed from needs_review to needs_work

Replying to davidloeffler:

This patch handles the "proof" argument in a slightly different way in order to make sure that bnfcertify isn't called unnecessarily.

I don't think this is quite right. I think the last two lines of the code for _ideal_class_log need to read

            self.__ideal_class_log[proof] = list(v[0])
            return self.__ideal_class_log[proof]

comment:14 Changed 11 years ago by davidloeffler

  • Status changed from needs_work to needs_review

Good point; thanks for spotting that. Here's a third attempt, which corrects the error as fwclarke suggests and also back-ports a doctest from #9359.

comment:15 Changed 11 years ago by fwclarke

Is there something wrong with the new doctest? Because

sage: K.<a> = NumberField(x^3 - x + 1) 
sage: K.class_number()
1

Changed 11 years ago by davidloeffler

replaces all previous attempts

comment:16 Changed 11 years ago by davidloeffler

Nothing wrong with the code, just my brain, apparently. It should have been

K.<a, b> = NumberField([x^3 - x + 1, x^2 + 26])

which has class group C_6 x C_3, but I copied and pasted the wrong lines, and the #random flag hid that. I've uploaded a fourth attempt with this correction.

Apologies for the complete mess I have been making of this ticket from start to finish.

comment:17 Changed 11 years ago by fwclarke

  • Status changed from needs_review to positive_review

It's fine now.

comment:18 Changed 11 years ago by mpatel

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