pexpect bug with Magma: conversion of rational polynomials

[kedlaya]

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

[kedlaya]

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.

[gh-mwageringel]

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]

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

[gh-mwageringel]

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`::

[git]

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

​109437b

Corrections to docstring

[kedlaya]

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.

[gh-mwageringel]

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.