Opened 11 years ago

Closed 11 years ago

#11586 closed defect (fixed)

bug in p-adic extension norm method

Reported by: William Stein Owned by: David Roe
Priority: major Milestone: sage-5.0
Component: padics Keywords: padic
Cc: Merged in: sage-5.0.beta10
Authors: David Roe Reviewers: Jennifer Balakrishnan
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #8241 Stopgaps:

Status badges

Description (last modified by Leif Leonhardy)

R.<x> = QQ[]
f = x^2 + 3*x + 1
K = Qp(7)
M.<a> = K.extension(f)

The above outputs 1 + O(7^20), which is completely wrong. The norm should be 7^2 + O(7^20), I think -- anyways, it should be divisible by 7.

The problem is that the actual code for the norm, which ones sees by typing a.norm?? assumes that the generator for M is in fact a generator for the maximal ideal of the ring of integers. However, this assumption is just completely false.

Typing a.norm??? we see

    norm_of_uniformizer = (-1)**self.parent().degree() * self.parent().defining_polynomial()[0]
    if self.valuation() == 0:
        return self.parent().ground_ring()(self.unit_part().matrix_mod_pn().det())
        return self.parent().ground_ring()(self.unit_part().matrix_mod_pn().det()) * norm_of_uniformizer**self.valuation()

The above is wrong as mentioned above. Moreover, it is nonoptimal in that norm_of_uniformizer is computed but never used in the case when self.valuation()==0.

This bug caused some confusion when computing with p-adic L-series for a research paper...

Apply 11586.patch to the Sage library.

Attachments (1)

11586.patch (1.9 KB) - added by David Roe 11 years ago.

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by David Roe

Attachment: 11586.patch added

comment:1 Changed 11 years ago by David Roe

Authors: David Roe
Dependencies: 8241
Keywords: padic added
Status: newneeds_review

Sorry about that. The attached patch should fix the problem (note that you need to apply #8241 first).

comment:2 Changed 11 years ago by Jennifer Balakrishnan

Status: needs_reviewpositive_review

Great; this fixes the bug. All tests pass!

comment:3 Changed 11 years ago by Jeroen Demeyer

Dependencies: 8241#8241
Reviewers: Jennifer Balakrishnan

comment:4 Changed 11 years ago by Leif Leonhardy

Description: modified (diff)

Make someone review #8241...

comment:5 Changed 11 years ago by Leif Leonhardy

Description: modified (diff)

comment:6 Changed 11 years ago by David Loeffler

Milestone: sage-4.7.2sage-pending

Isn't this what we have a milestone "sage-pending" for?

comment:7 in reply to:  6 Changed 11 years ago by Leif Leonhardy

Replying to davidloeffler:

Isn't this what we have a milestone "sage-pending" for?

I personally don't use "sage-pending" for that situation, since my script does dependency checking, so won't attempt to merge it, but considers it for inclusion, i.e., I'll see that it has positive review, and all of its dependencies with their state.

My interpretation (or use) of "sage-pending" is to postpone tickets for other reasons, so I currently only see that they're postponed, and don't consider them further.

I thought this was the original intent, to have some "formal" way to plan which tickets get merged when during management of a specific release, also since we don't have milestones for devel releases.

comment:8 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-pendingsage-5.0

comment:9 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta10
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.