Opened 22 months ago
Closed 21 months ago
#30341 closed defect (fixed)
pexpect bug with Magma: conversion of rational polynomials
Reported by:  kedlaya  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description (last modified by )
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
Change History (11)
comment:1 Changed 22 months ago by
 Description modified (diff)
comment:2 Changed 21 months ago by
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
 Branch set to u/kedlaya/magma_conversion_of_rational_polynomials
comment:4 Changed 21 months ago by
 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:
dba1ba0  Turn on preparsing for univariate polynomials

comment:5 Changed 21 months ago by
 Status changed from new to needs_review
comment:6 Changed 21 months ago by
comment:7 Changed 21 months ago by
 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
 Commit changed from dba1ba065597b748d8820650d35b897de38fbbbd to 109437bb43a0fc53da99ee5f07456263a98cab60
Branch pushed to git repo; I updated commit sha1. New commits:
109437b  Corrections to docstring

comment:9 followup: ↓ 10 Changed 21 months ago by
 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
 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
 Branch changed from u/kedlaya/magma_conversion_of_rational_polynomials to 109437bb43a0fc53da99ee5f07456263a98cab60
 Resolution set to fixed
 Status changed from positive_review to closed
It looks like Magma is a red herring; the issue is in
sage.misc.sage_eval.sage_eval
. Thesage
method reads as follows:In this case,
preparse
is coming backfalse
and this is the trouble: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
.