pexpect bug with Magma: conversion of rational polynomials

kedlaya commented 4 years ago

It seems that the pexpect interface to Magma does not correctly handle multiprecision integers. A nearly minimal example:

sage: P.<t> = PolynomialRing(QQ)
sage: l = [-27563611963/4251528, -48034411/104976, -257/54, 1]
sage: u1 = P(l)
sage: u2 = P(magma(u1).sage())
sage: u1 == u2
False
sage: u1                                                                                                                                      
t^3 - 257/54*t^2 - 48034411/104976*t - 27563611963/4251528
sage: u2                                                                                                                                      
t^3 - 257/54*t^2 - 48034411/104976*t - 8389474481/1294028

Note this issue does not arise with the numbers themselves:

sage: magma(l).sage() == l                                                                                                                    
True

Component: interfaces

Keywords: Magma

Author: Kiran Kedlaya

Branch/Commit: 109437b

Reviewer: Markus Wageringel

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

kedlaya commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,11 @@
 It seems that the pexpect interface to Magma does not correctly handle multiprecision integers. A nearly minimal example:

+sage: P. = PolynomialRing(QQ) sage: l = [-27563611963/4251528, -48034411/104976, -257/54, 1] -sage: u1 = P(l)
-sage: u2 = P(magma(u1).sage())
-sage: u1 == u2
+sage: u1 = P(l) +sage: u2 = P(magma(u1).sage()) +sage: u1 == u2 False sage: u1
t^3 - 257/54t^2 - 48034411/104976t - 27563611963/4251528

kedlaya commented 4 years ago

comment:1

It looks like Magma is a red herring; the issue is in sage.misc.sage_eval.sage_eval. The sage method reads as follows:

        z, preparse = self.Sage(nvals=2)
        s = str(z)
        preparse = str(preparse) == 'true'
        return sage.misc.sage_eval.sage_eval(s, preparse=preparse)

In this case, preparse is coming back false and this is the trouble:

sage: s = magma(u1).Sage(); s                                                   
QQ['t'.replace('$.', 'x').replace('.', '')]([ -27563611963/4251528, -48034411/104976, -257/54, 1 ])
sage: sage.misc.sage_eval.sage_eval(str(s), preparse=False)                     
t^3 - 257/54*t^2 - 48034411/104976*t - 8389474481/1294028
sage: sage.misc.sage_eval.sage_eval(str(s), preparse=True)                      
t^3 - 257/54*t^2 - 48034411/104976*t - 27563611963/4251528

Is there any reason not to just force preparsing in all cases? It doesn't seem to break any doctests in sage/interfaces/magma.py.

mwageringel commented 4 years ago

comment:2

I do not have access to Magma, but the code in src/sage/ext_data/magma/sage/basic.m seems to make an effort to decide whether preparsing should be used or not (I assume there is some reason for this). It does not look like this can work consistently, though. For example, the code for converting polynomials appears to ignore the preparsing flag for the list of coefficients, which leads to the bug.

intrinsic Sage(X::RngUPolElt) -> MonStgElt, BoolElt
{}
  return Sprintf("%o(%o)", Sage(Parent(X)), Sage(Coefficients(X))), false;
end intrinsic;

Enabling preparsing sounds like a good idea to me if it works for all the intended cases in basic.m. This would reduce a lot of complexity.

kedlaya commented 4 years ago

Branch: u/kedlaya/magma_conversion_of_rational_polynomials

kedlaya commented 4 years ago

Commit: dba1ba0

kedlaya commented 4 years ago

comment:4

I'm not sure why preparsing is enabled in some cases but not others.

In this commit I made a minimal change to fix this particular bug: for univariate polynomials, I take the preparse flag from the base ring (which is true except for ordinary integers).

New commits:

dba1ba0 Turn on preparsing for univariate polynomials

kedlaya commented 4 years ago

Author: Kiran Kedlaya

mwageringel commented 4 years ago

comment:7

This looks ok to me, as the same approach is used for matrices. Have you tried whether the conversion for multivariate polynomials works as well?

Two small things: the tests need to be tagged with the optional - magma tag, and the following syntax is used to refer to trac tickets:

-        Tests for `trac`:30341::
+        Tests for :trac:`30341`::

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

109437b Corrections to docstring

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from dba1ba0 to 109437b

kedlaya commented 4 years ago

comment:9

There was already a doctest in the previous commit about multivariate polynomials. No change was needed for them.

I made the requested changes to the docstring. Adding the optional tags should fix some (all?) of the patchbot errors.

mwageringel commented 4 years ago

Reviewer: Markus Wageringel

mwageringel commented 4 years ago

comment:10

Replying to @kedlaya:

There was already a doctest in the previous commit about multivariate polynomials. No change was needed for them.

Indeed, I had missed that.

The bots are (morally) green now and the changes look good to me. I trust that you have tried this with Magma, so I am setting this to positive.

vbraun commented 4 years ago

Changed branch from u/kedlaya/magma_conversion_of_rational_polynomials to 109437b

sagemath / sage

pexpect bug with Magma: conversion of rational polynomials #30341