Warn when creating number fields with non-monic-integral polynomials

[sagetrac-Bouillaguet]

Add warnings for the problem at #252.

Apply:

• attachment: 14146_number_fields.patch
• attachment: 14146_reviewer.patch
• attachment: 14146-doctest.patch

Component: number fields

Keywords: pari, monic, integral, integer

Author: Charles Bouillaguet

Reviewer: Marco Streng

Merged: sage-5.10.beta2

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

[jdemeyer]

comment:2

Duplicate of #252.

[sagetrac-Bouillaguet]

comment:4

Jeroen, I suggest the following : let's keep #252 open, because the problem is not fixed (I will take a look at it). In the meantime, let me post a patch here which at least prints a warning when the user runs into this problem (i.e. when a number field with a non-monic or non-integral polynomial is created). I am not the first one to hit this problem, and just printing a warning (or failing more gracefully) would help users.

[mstreng]

comment:7

Thanks Charles, this is very good to have!

A comment and a question:

• The doctest with output "doctest:311" will likely break later on. Better is to write "doctest:..." in the output.

• Why an error in one situation and a warning in another? Why not a warning in both? You are disallowing some functionality with the error, as seen from the following in an unpatched Sage 5.7:

sage: k.<i> = NumberField(x^2+1)
sage: l.<b> = k.extension(5*x^2 + 3)
sage: c = b + b
sage: c^2 == -12/5
True
sage: c^2 == b
False

• There is a discrepancy between the absolute and the relative case. With your patch on Sage 5.7:

sage: k.<a> = NumberField(5*x^2+3)
NotImplementedError: number fields for non-monic polynomials not yet implemented.
sage: k.<i> = NumberField(x^2+1)
sage: l.<b> = k.extension(5*x^2+3)
ValueError: the polynomial must be monic

I prefer the NotImplementedError, and it would be good to add a reference to #252 to that error message.

[jdemeyer]

comment:8

I agree with Marco's comments.

And don't forget to add your names as Author/Reviewer.

[mstreng]

Author: Charles Bouillaguet

[mstreng]

Reviewer: Marco Streng

[mstreng]

Changed keywords from pari to pari, monic, integral, integer

[mstreng]

comment:11

Replying to @sagetrac-Bouillaguet:

I did not touch the absolute case.

Fine with me. I looked at this in a bit more detail, and have some questions:

• Why import "warn" from "sage.misc.superseded" and not directly from Python's "warnings" module as is done in superseded.py?

• When I try the examples in a Sage session, I get the warning printed twice. Is that supposed to happen?

sage: k.<a> = NumberField(x^2+1)
sage: l.<b> = k.extension(x^3-1/2)
/sage-5.6/local/lib/python2.7/site-packages/sage/rings/number_field/number_field_rel.py:307: UserWarning: PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors. See https://github.com/sagemath/sage-prod/issues/252
  warn("PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors. See https://github.com/sagemath/sage-prod/issues/252")

[mstreng]

comment:12

Replying to @mstreng:

have some questions:

And one more: do you have a reason for keeping the commented warning in there?

[sagetrac-Bouillaguet]

Attachment: 14146_number_fields.patch.gz

[sagetrac-Bouillaguet]

comment:13

Replying to @mstreng:

Replying to @mstreng:

have some questions:

And one more: do you have a reason for keeping the commented warning in there?

nope :) I updated the patch

[mstreng]

comment:14

Replying to @sagetrac-Bouillaguet:

nope :) I updated the patch

Thanks! And what about the earlier questions?

[mstreng]

comment:15

Attachment: 14146_reviewer.patch.gz

I did some of the corrections myself that I did not get an answer to:

• "..." instead of a doctest number
• NotImplementedError, because it is more appropriate than ValueError and also more in line with the absolute case
• import warn more directly

However, I still don't understand why I get the warning printed twice:

sage: k.<a> = NumberField(x^2+1)
sage: l.<b> = k.extension(x^3-1/2)
/sage-5.6/local/lib/python2.7/site-packages/sage/rings/number_field/number_field_rel.py:307: UserWarning: PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors. See https://github.com/sagemath/sage-prod/issues/252
  warn("PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors. See https://github.com/sagemath/sage-prod/issues/252")

[mstreng]

comment:16

I know why the warning is printed twice: the line that caused the warning is printed, after the warning is printed. You can specify how deep in the stack this line needs to be found:

Without specifying stacklevel or when giving stacklevel=1:

/Users/marcostreng/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/number_field/number_field_rel.py:310: UserWarning: PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors
  warn("PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors", stacklevel=1)

stacklevel=2:

/Users/marcostreng/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py:4116: UserWarning: PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors
  return NumberField_relative(self, poly, str(name), check=check, embedding=embedding)

stacklevel=3:

/Users/marcostreng/sage-5.9.beta1/local/bin/sage-ipython:1: UserWarning: PARI only handles integral absolute polynomials. Computations in this field might trigger PARI errors
  #!/usr/bin/env python

Anyway, no value of stacklevel seemed to provide me with anything useful for .py or .sage files or the IPython console, so we might as well keep it as it is.

[mstreng]

comment:17

All tests pass. Positive review to attachment: 14146_number_fields.patch except for what I changed in the reviewer patch. Could someone (Charles?) review my reviewer patch? Thanks!

[sagetrac-Bouillaguet]

comment:18

The reviewer patch is fine. Still, the example given above by Marco is now disallowed. This one :

sage: k.<i> = NumberField(x^2+1)
sage: l.<b> = k.extension(5*x^2 + 3)
sage: c = b + b
sage: c^2 == -12/5
True
sage: c^2 == b
False

[mstreng]

comment:19

Thanks!

Replying to @sagetrac-Bouillaguet:

the example given above by Marco is now disallowed

I know, it is too bad. I justified it to myself by noting that it is in line with the absolute case.

[jdemeyer]

comment:20

This causes a --long doctest failure in devel/sage/sage/schemes/elliptic_curves/gal_reps.py

[mstreng]

Attachment: 14146-doctest.patch.gz

[mstreng]

comment:22

Replying to @jdemeyer:

This causes a --long doctest failure in devel/sage/sage/schemes/elliptic_curves/gal_reps.py

Sorry about that. Is testing the whole sage library on shared machines safe again? I relied on patchbot ever since the warnings on sage-devel, and patchbot does not do long tests.

[jdemeyer]

comment:23

Replying to @mstreng:

Replying to @jdemeyer:

This causes a --long doctest failure in devel/sage/sage/schemes/elliptic_curves/gal_reps.py

Sorry about that. Is testing the whole sage library on shared machines safe again?

Safe again, what do you mean? What has been unsafe?

[mstreng]

comment:24

Replying to @jdemeyer:

Safe again, what do you mean? What has been unsafe?

#13579, looks like it's fixed, so I suppose that answers my question

[jdemeyer]

comment:25

Yes, that has been fixed a while ago.

[mstreng]

comment:26

Replying to @jdemeyer:

Yes, that has been fixed a while ago.

Thanks, sorry for not running long tests. I did now, and all tests pass. Only attachment: 14146-doctest.patch needs review.

[jdemeyer]

Merged: sage-5.10.beta2