Opened 19 months ago

Closed 9 months ago

#29364 closed defect (fixed)

Bug in S-class groups of number fields

Reported by: gh-bmatschke Owned by:
Priority: major Milestone: sage-9.3
Component: number fields Keywords: S_class_group, fractional_ideal
Cc: Merged in:
Authors: Dave Morris Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 677111e (Commits, GitHub, GitLab) Commit: 677111e60524cc2e8a37deb31d766a72182bdefe
Dependencies: Stopgaps:

Status badges

Description

Coercing a fractional ideal of a number field to an S-class group over the same number field sometimes raises a KeyError? and an AssertionError?.

Here is a failing example:

R.<x> = QQ[]
L.<t> = NumberField(x^2 - 6058)
S = L.primes_above(2)
C = L.S_class_group(S)
J = L.fractional_ideal(1)
print("C(J):", C(J))	# Should print the trivial S-ideal class of C, but raises errors.

This was tested in Sage 9.0, 8.8, and 8.6, always yielding the same error.

Change History (11)

comment:1 Changed 17 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

comment:2 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 10 months ago by gh-DaveWitteMorris

  • Branch set to public/29364

comment:4 Changed 10 months ago by gh-DaveWitteMorris

  • Authors changed from Benjamin Matschke to Dave Morris
  • Commit set to 0241a7422f63e8c6900e37db3f6f10c0da995a05
  • Status changed from new to needs_review

The bug is in the NumberField._S_class_group_quotient_matrix() method.

sage: R.<x> = QQ[] 
....: L.<t> = NumberField(x^2 - 6058) 
....: S = L.primes_above(2) 
....: L._S_class_group_quotient_matrix(tuple(S))                                
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
   ...
During handling of the above exception, another exception occurred:

AssertionError                            Traceback (most recent call last)
    ...
/Applications/math/sage-9.2/local/lib/python3.7/site-packages/sage/rings/number_field/number_field.py in _S_class_group_quotient_matrix(self, S)
   4644         M = matrix(ZZ, M)
   4645         A, Q = M.hermite_form(transformation=True)
-> 4646         assert A[:c] == 1 and A[c:] == 0
   4647         return Q[:c, :a]
   4648 

AssertionError:

The rows of the matrix M are generators of <S> and generators of the quotient I/<S>, where I is the ideal class group, so the rows generate I. The algorithm assumed that lifting these generators to Zn would provide a set of generators of Zn, but that is not usually true.

The PR adds additional rows to M that are generators of the kernel of the homomorphism from Zn to I. (Each generator of the kernel has only one nonzero entry, which is the order of the corresponding generator of I.) This squashes the bug:

sage: R.<x> = QQ[] 
....: L.<t> = NumberField(x^2 - 6058) 
....: S = L.primes_above(2) 
....: C = L.S_class_group(S) 
....: J = L.fractional_ideal(1) 
....: print("C(J):", C(J))                                                               
C(J): Trivial S-ideal class

(The fix is only one line of code, but the PR also adds a doctest, and deletes some space-only lines elsewhere in the file.)


New commits:

0241a74fix trac 29364 S-class group

comment:5 Changed 10 months ago by chapoton

doctest failure in src/sage/rings/number_field/number_field.py, see patchbot

comment:6 Changed 10 months ago by chapoton

and the syntax for the trac link is plain wrong

comment:7 Changed 10 months ago by git

  • Commit changed from 0241a7422f63e8c6900e37db3f6f10c0da995a05 to 677111e60524cc2e8a37deb31d766a72182bdefe

Branch pushed to git repo; I updated commit sha1. New commits:

677111ereviewer corrections

comment:8 Changed 10 months ago by gh-DaveWitteMorris

Oops! Thanks for pointing out that I goofed up the trac link.

I was surprised that the patchbot failed the doctest that I wrote. (It works fine on my computer.) But the matrix is not unique, so maybe it's more surprising that the other doctests of this method don't fail. Anyway, I wrote a new doctest that checks whether the matrix is valid, rather than requiring a certain value. I hope the patchbots will be happy with this one.

comment:9 Changed 10 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be (not a mathematical review, though)

comment:10 Changed 9 months ago by gh-DaveWitteMorris

Thanks!

comment:11 Changed 9 months ago by vbraun

  • Branch changed from public/29364 to 677111e60524cc2e8a37deb31d766a72182bdefe
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.