Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8722 closed defect (fixed)

S-units sometimes broken and sometimes just plain wrong for relative fields

Reported by: davidloeffler Owned by: davidloeffler
Priority: major Milestone: sage-4.4
Component: number fields Keywords:
Cc: Merged in: sage-4.4.alpha2
Authors: David Loeffler Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by davidloeffler)

The code for S-unit groups of number fields calls the degree method. For relative number fields this deliberately returns an error, because of the ambiguity between absolute and relative degree.

sage: L.<a,b> = NumberField([x^2 + 1, x^2 - 5])
sage: sage: p = L.ideal((-1/2*b - 1/2)*a + 1/2*b - 1/2)
sage: sage: W = L.S_units([p]); W
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
...
NotImplementedError: For a relative number field you must use relative_degree or absolute_degree as appropriate

In this case I think it should be absolute_degree, but changing this returns wrong output:

sage: L.<a,b> = NumberField([x^2 + 1, x^2 - 5])
sage: p = L.ideal((-1/2*b - 1/2)*a + 1/2*b - 1/2)
sage: p.absolute_norm()
9
sage: p.is_prime()
True
sage: W = L.S_units([p]); W
[1/2*a + 7/4, a, 1/2*b - 1/2]
sage: W[0].valuation(L.primes_above(2)[0])
-4

So the first element of the list of S-units isn't actually an S-unit!

In other examples the code just blows up, because it calls residue_field and that dies because of #8721:

sage: L.<a, b> = NumberField([polygen(QQ)^2 - 3, polygen(QQ)^2 - 5])
sage: L.S_units([L.ideal(a)])

This is arguably less bad: raising an error is far better than silently a wrong answer.

Attachments (1)

trac_8722.patch (1.7 KB) - added by davidloeffler 10 years ago.
apply over patches at #8446

Download all attachments as: .zip

Change History (6)

comment:1 Changed 10 years ago by davidloeffler

  • Description modified (diff)

comment:2 Changed 10 years ago by davidloeffler

  • Status changed from new to needs_review

Here's a patch. Turns out that the code was using K.gen and the correct answer is to call K.absolute_generator, which isn't the same in the above example. This fixes the first example; the second is an instance of #8721.

Changed 10 years ago by davidloeffler

apply over patches at #8446

comment:3 Changed 10 years ago by cremona

  • Status changed from needs_review to positive_review

Looks good, applied fine to 4.4.alpha0 + #8446 patches, and all tests in sage/rings/number_field pass.

Positive review!

comment:4 Changed 10 years ago by jhpalmieri

  • Authors set to David Loeffler
  • Merged in set to sage-4.4.alpha2
  • Resolution set to fixed
  • Reviewers set to John Cremona
  • Status changed from positive_review to closed

Merged into 4.4.alpha2.

comment:5 Changed 10 years ago by mvngu

  • Milestone changed from sage-5.0 to sage-4.4
Note: See TracTickets for help on using tickets.