Opened 9 years ago

Closed 8 years ago

#11586 closed defect (fixed)

bug in p-adic extension norm method

Reported by: was Owned by: roed
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:

Description (last modified by leif)

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

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())
    else:
        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 roed 9 years ago.

Download all attachments as: .zip

Change History (10)

Changed 9 years ago by roed

comment:1 Changed 9 years ago by roed

  • Authors set to David Roe
  • Dependencies set to 8241
  • Keywords padic added
  • Status changed from new to needs_review

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

comment:2 Changed 9 years ago by jen

  • Status changed from needs_review to positive_review

Great; this fixes the bug. All tests pass!

comment:3 Changed 9 years ago by jdemeyer

  • Dependencies changed from 8241 to #8241
  • Reviewers set to Jennifer Balakrishnan

comment:4 Changed 9 years ago by leif

  • Description modified (diff)

Make someone review #8241...

comment:5 Changed 9 years ago by leif

  • Description modified (diff)

comment:6 follow-up: Changed 9 years ago by davidloeffler

  • Milestone changed from sage-4.7.2 to sage-pending

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

comment:7 in reply to: ↑ 6 Changed 9 years ago by leif

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 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.0

comment:9 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta10
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.