Opened 3 years ago

Closed 2 years ago

#20244 closed defect (fixed)

root_field() does not work for p-adic fields

Reported by: saraedum Owned by:
Priority: major Milestone: sage-7.2
Component: padics Keywords: days71, beginner, sd87
Cc: Merged in:
Authors: Immi Halupczok Reviewers: David Roe, Aly Deines
Report Upstream: N/A Work issues:
Branch: 66ba958 (Commits) Commit: 66ba9582ed3226655276322c368bfdaa5687552b
Dependencies: #20254 Stopgaps:

Description (last modified by saraedum)

Currently it creates a quotient of a polynomial ring over the p-adics. It should call extension() to create a proper extension instead.

sage: R.<x> = Qp(3)[]
sage: (x^3+3).root_field('a')
Univariate Quotient Polynomial Ring in a over 3-adic Field with capped relative precision 20 with modulus (1 + O(3^20))*x^3 + (3 + O(3^21))

Also the documentation is somewhat confusing. It should be made clear that this is not the same as splitting_field().

Change History (15)

comment:1 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:2 Changed 3 years ago by immi

  • Branch set to u/immi/root_field___does_not_work_for_p_adic_fields

comment:3 Changed 3 years ago by immi

  • Authors set to Immi Halupczok
  • Commit set to 66ba9582ed3226655276322c368bfdaa5687552b
  • Status changed from new to needs_review

New commits:

66ba958fixed root_field for polynomials over Qp

comment:4 Changed 3 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

Looks good. I doubt that anywhere else in Sage was using root_field for p-adic polynomials, since polynomial quotient rings have so little functionality.

comment:5 Changed 3 years ago by wuthrich

Maybe, in to avoid rewriting doctest twice, it would be good to put this in after #20254.

comment:6 Changed 3 years ago by roed

Why not have #20254 depend on this? With this this change, using root_field should be perfectly fine.

comment:7 Changed 3 years ago by wuthrich

If I were to do both I would do it the other way around to save me the work of changing the doctests in padic_lseries twice. But if this is urgent, then just change the output there and I adapt mine on top of this.

comment:8 Changed 3 years ago by roed

I don't understand why you would need to change the doctests in padic_lseries twice. Just because root_field works now doesn't mean you need to use it....

Can you push a branch to #20254 so I can look at the doctests you're referring to?

comment:9 Changed 3 years ago by wuthrich

if you change root_field the printing of the p-adic L-functions of elliptic curves will change. I found some errors and they will actually change by some factor 2, too. So they will be changed twice.

It doesn't matter. By now I have written more then the double change will cause :)

comment:10 Changed 3 years ago by vbraun

sage -t --long src/sage/misc/cachefunc.pyx  # 1 doctest failed
sage -t --long src/sage/schemes/elliptic_curves/padic_lseries.py  # 7 doctests failed
sage -t --long src/sage/schemes/elliptic_curves/sha_tate.py  # 1 doctest failed

comment:11 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:12 Changed 3 years ago by wuthrich

  • Dependencies set to #20254

I believe that applying this ticket after #20254 should get rid of the doctest problems found above. I will run the test later today.

comment:13 Changed 2 years ago by roed

  • Keywords sd87 added

comment:14 Changed 2 years ago by aly.deines

  • Reviewers changed from David Roe to David Roe, Aly Deines
  • Status changed from needs_work to positive_review

The doctests that failed above now pass (all doctest now pass.)

comment:15 Changed 2 years ago by vbraun

  • Branch changed from u/immi/root_field___does_not_work_for_p_adic_fields to 66ba9582ed3226655276322c368bfdaa5687552b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.