Opened 12 years ago

Closed 12 years ago

## #9244 closed defect (fixed)

# Number field class group improvements

Reported by: | David Loeffler | Owned by: | David Loeffler |
---|---|---|---|

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 12 years ago by

Milestone: | sage-4.4.5 → sage-4.5 |
---|

### comment:2 Changed 12 years ago by

Status: | new → 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 12 years ago by

Status: | needs_review → 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 12 years ago by

Status: | needs_work → 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 12 years ago by

Authors: | → David Loeffler |
---|---|

Description: | modified (diff) |

Reviewers: | → John Cremona, Jim Stankewicz |

### comment:6 Changed 12 years ago by

Reviewers: | John Cremona, Jim Stankewicz → Francis Clarke, John Cremona, Jim Stankewicz |
---|---|

Status: | needs_review → 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 12 years ago by

Status: | needs_work → 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 12 years ago by

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

### comment:9 Changed 12 years ago by

Status: | needs_review → 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 12 years ago by

Status: | positive_review → 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 12 years ago by

Status: | needs_work → 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 12 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 Changed 12 years ago by

Status: | needs_review → 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 12 years ago by

Status: | needs_work → 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 12 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 12 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:18 Changed 12 years ago by

Merged in: | → sage-4.5.2.alpha0 |
---|---|

Resolution: | → fixed |

Status: | positive_review → closed |

**Note:**See TracTickets for help on using tickets.

Milestone sage-4.4.5 deleted