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: |
Description (last modified by )
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)
Change History (19)
comment:1 Changed 11 years ago by
- Milestone changed from sage-4.4.5 to sage-4.5
comment:2 Changed 11 years ago by
- 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
- 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
- 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
- Description modified (diff)
- Reviewers set to John Cremona, Jim Stankewicz
comment:6 Changed 11 years ago by
- 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
- 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: ↓ 9 Changed 11 years ago by
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
- 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
- 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: ↓ 13 Changed 11 years ago by
- 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
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
- 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
- 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
Is there something wrong with the new doctest? Because
sage: K.<a> = NumberField(x^3 - x + 1) sage: K.class_number() 1
comment:16 Changed 11 years ago by
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
- Status changed from needs_review to positive_review
It's fine now.
comment:18 Changed 11 years ago by
- Merged in set to sage-4.5.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Milestone sage-4.4.5 deleted