Opened 2 years ago

Closed 2 years ago

#22936 closed defect (fixed)

Newton polygon of polynomials over power series with infinite precision

Reported by: bruno Owned by:
Priority: major Milestone: sage-8.0
Component: algebra Keywords: newton polygon
Cc: caruso Merged in:
Authors: Xavier Caruso Reviewers: Bruno Grenet, Julian Rüth
Report Upstream: N/A Work issues:
Branch: f5fbc72 (Commits) Commit: f5fbc720f82b11787301b36efe8359c7e1dc653b
Dependencies: Stopgaps:

Description

Computing the Newton polygon of a polynomial with coefficients in a ring of power series fails if the coefficients have infinite precision: 

sage: S.<x> = PowerSeriesRing(GF(5))
sage: R.<y> = S[]
sage: p = x^2+y+x*y^2
sage: p.newton_polygon()
Traceback (most recent call last):
...
IndexError: list index out of range

Change History (10)

comment:1 Changed 2 years ago by caruso

  • Branch set to u/caruso/newton_polygon_infinite_prec

comment:2 Changed 2 years ago by caruso

  • Commit set to 8204e97c49c9ef9b1dad0d3b20c3a2cceb0be199
  • Status changed from new to needs_review

I attach a small patch fixing this issue. I've also updated the method _factor_of_degree in order to make the slope factorization work correctly. (Otherwise, at infinite precision, it may never stop.)

Last edited 2 years ago by caruso (previous) (diff)

comment:3 Changed 2 years ago by git

  • Commit changed from 8204e97c49c9ef9b1dad0d3b20c3a2cceb0be199 to 61cb00ccf1648775300d83c7896ae058a51b0ebb

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

​61cb00cNewton polygon and slope factorization fixed

comment:4 Changed 2 years ago by git

  • Commit changed from 61cb00ccf1648775300d83c7896ae058a51b0ebb to ac0d97598c09c76e7a7b8ccf56e4bad9d6a1ce5a

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

​ac0d975Replace TESTS: by TESTS::

comment:5 Changed 2 years ago by saraedum

Looks good, except for the TESTS:. Positive review if the patchbot is happy.

New commits:

​ac0d975Replace TESTS: by TESTS::

comment:6 Changed 2 years ago by bruno

  • Status changed from needs_review to needs_work

Two One comments:

  • You changed too many TESTS: by TESTS::: There should be double colons only when TESTS: is followed by an actual test, not when it is followed by a sentence that explains the test.
  • It would be nice to add a test for slope factorization. (The test already exists...)
Last edited 2 years ago by bruno (previous) (diff)

comment:7 Changed 2 years ago by git

  • Commit changed from ac0d97598c09c76e7a7b8ccf56e4bad9d6a1ce5a to f5fbc720f82b11787301b36efe8359c7e1dc653b

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

​144898dMerge branch 'newton_polygon_infinite_prec' into a
​f5fbc72Better writing for TESTS::?

comment:8 Changed 2 years ago by caruso

  • Status changed from needs_work to needs_review

comment:9 Changed 2 years ago by bruno

  • Authors set to Xavier Caruso
  • Reviewers set to Bruno Grenet, Julian Rüth
  • Status changed from needs_review to positive_review

LGTM, thanks for the patch!

comment:10 Changed 2 years ago by vbraun

  • Branch changed from u/caruso/newton_polygon_infinite_prec to f5fbc720f82b11787301b36efe8359c7e1dc653b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.