Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UserWarning when factoring polys over NumberFields #27765

Closed
NathanDunfield opened this issue May 4, 2019 · 14 comments
Closed

UserWarning when factoring polys over NumberFields #27765

NathanDunfield opened this issue May 4, 2019 · 14 comments

Comments

@NathanDunfield
Copy link

For a number field K whose defining polynomial has a non-integral rational coefficient, factoring a polynomial with coefficients in K sometimes results in the following UserWarning:

SageMath version 8.8.beta1, Release Date: 2019-04-07
sage: K.<a> = NumberField(x^2 - 1/2)
sage: R.<y> = PolynomialRing(K)
sage: p = 11*a*y + 7
sage: p.factor()
/sage/local/lib/python3.7/site-packages/sage/rings/number_field/number_field.py:1682: UserWarning: interpreting PARI polynomial 11 relative to the defining polynomial x^2 - 2 of the PARI number field
  % (x, self.pari_polynomial()))
(11*a) * (y + 14/11*a)

This is related to #22202 and see https://groups.google.com/forum/#!topic/sage-devel/-4B4322qI9M for further discussion.

CC: @videlec @jdemeyer @unhyperbolic @culler

Component: number fields

Author: Nathan Dunfield

Branch/Commit: 5b89a97

Reviewer: Matthias Goerner

Issue created by migration from https://trac.sagemath.org/ticket/27765

@NathanDunfield
Copy link
Author

comment:1

I think I have a reasonable solution to this: _element_constructor_ shouldn't raise the warning if the polynomial it's given is constant since that case is unambiguous. Writing patch now...

@NathanDunfield
Copy link
Author

New commits:

d064855NumberField._element_constructor_: when given a constant PARI t_POL, there's no ambiguity and hence no need to warn the user

@NathanDunfield
Copy link
Author

Commit: d064855

@NathanDunfield
Copy link
Author

Branch: u/dunfield/nf_warning

@NathanDunfield NathanDunfield self-assigned this May 4, 2019
@NathanDunfield
Copy link
Author

comment:3

Ok, I think that does it.

@unhyperbolic
Copy link

comment:4

Thanks!

@unhyperbolic
Copy link

comment:5

I am so sorry for re-opening this. But I was just playing around some more with pari and discovered the following:

>>> (pari('y^4')+pari('x^2')-pari('x^2')).poldegree()
0
>>> (pari('y^4')+pari('x^2')-pari('x^2'))[0]
y^4

I am not sure how much of a corner case this is and that there is actually additional work necessary. But I definitively want to think this over before giving it a positive review.

@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:6

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jun 14, 2019
@NathanDunfield
Copy link
Author

comment:7

Replying to @unhyperbolic:

I am so sorry for re-opening this. But I was just playing around some more with pari and discovered the following:

Hmm, Pari's handling of multivariable polynomials is pretty idiosyncratic:

sage: p = pari('x^2 + y^4')
sage: q = pari('x^2')
sage: p.variables(), q.variables()
([x, y], [x])
sage: a = p - p + 1
sage: a, a.poldegree(), a.variables()
(1, 0, [x])
sage: b = q - q
sage: b, b.poldegree(), b.variables()
(0, -9223372036854775807, [x])
sage: c = p - q
sage: c, c.poldegree(), c.variables()
(y^4, 0, [x, y])
sage: pari('z - z').variables()
[z]
sage: pari('z + x - z - x').variables()
[x]

I think the following is a better test for being constant:

sage: def degrees(poly): return [poly.poldegree(v) for v in poly.variables()]
sage: degrees(a)
[0]
sage: degrees(b)
[-9223372036854775807]
sage: degrees(c)
[0, 4]
sage: def is_const(poly): 
          return all(poly.poldegree(v) <= 0 for v in poly.variables())
sage: [is_const(x) for x in [a, b, c, p, q]]
[True, True, False, False, False]

I will update my branch along these lines.

@NathanDunfield NathanDunfield modified the milestones: sage-8.9, sage-8.8 Jun 23, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 24, 2019

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

f63c289Merge branch 'master' into t/27765/nf_warning
5b89a97Improved test for constant polynomial.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 24, 2019

Changed commit from d064855 to 5b89a97

@unhyperbolic
Copy link

comment:10

Thanks!

@NathanDunfield
Copy link
Author

Reviewer: Matthias Goerner

@fchapoton fchapoton modified the milestones: sage-8.8, sage-8.9 Jul 1, 2019
@vbraun
Copy link
Member

vbraun commented Jul 7, 2019

Changed branch from u/dunfield/nf_warning to 5b89a97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants