#30341 closed defect (fixed)

pexpect bug with Magma: conversion of rational polynomials

Reported by: kedlaya Owned by:
Priority: major Milestone: sage-9.2
Component: interfaces Keywords: Magma
Cc: Merged in:
Authors: Kiran Kedlaya Reviewers: Markus Wageringel
Report Upstream: N/A Work issues:
Branch: 109437b (Commits, GitHub, GitLab) Commit: 109437bb43a0fc53da99ee5f07456263a98cab60
Dependencies: Stopgaps:

Status badges

Description (last modified by 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
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                                                                                                                    

Change History (11)

comment:1 Changed 22 months ago by kedlaya

  • Description modified (diff)

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.

comment:2 Changed 21 months ago by 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.

comment:3 Changed 21 months ago by kedlaya

  • Branch set to u/kedlaya/magma_conversion_of_rational_polynomials

comment:4 Changed 21 months ago by kedlaya

  • Commit set to dba1ba065597b748d8820650d35b897de38fbbbd

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:

dba1ba0Turn on preparsing for univariate polynomials

comment:5 Changed 21 months ago by kedlaya

  • Status changed from new to needs_review

comment:6 Changed 21 months ago by kedlaya

  • Authors set to Kiran Kedlaya

comment:7 Changed 21 months ago by gh-mwageringel

  • Status changed from needs_review to needs_work

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

comment:8 Changed 21 months ago by git

  • Commit changed from dba1ba065597b748d8820650d35b897de38fbbbd to 109437bb43a0fc53da99ee5f07456263a98cab60

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

109437bCorrections to docstring

comment:9 follow-up: Changed 21 months ago by kedlaya

  • Status changed from needs_work to needs_review

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.

comment:10 in reply to: ↑ 9 Changed 21 months ago by gh-mwageringel

  • Reviewers set to Markus Wageringel
  • Status changed from needs_review to positive_review

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.

comment:11 Changed 21 months ago by vbraun

  • Branch changed from u/kedlaya/magma_conversion_of_rational_polynomials to 109437bb43a0fc53da99ee5f07456263a98cab60
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.