Opened 6 years ago

Closed 3 years ago

Last modified 22 months ago

#21020 closed enhancement (fixed)

gap/libgap polynomial to Sage

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.7
Component: interfaces Keywords: libgap
Cc: dimpase, tscrim Merged in:
Authors: Frédéric Chapoton, Vincent Delecroix Reviewers: Vincent Delecroix, Frédéric Chapoton, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 9437416 (Commits, GitHub, GitLab) Commit:
Dependencies: #27021 Stopgaps:

Status badges

Description

Given a polynomial in gap/libgap it is (most of the time) not possible to get it back into Sage.

Note: the other direction is dealt with in #18266

Change History (36)

comment:1 Changed 6 years ago by dimpase

  • Cc dimpase added

comment:2 Changed 6 years ago by tscrim

  • Cc tscrim added

comment:3 follow-up: Changed 3 years ago by chapoton

  • Branch set to u/chapoton/21020
  • Commit set to 46e6f88e14b7d3816d7aad194e83d26d80910e94

first tentative:

  • working conversion of polynomial rings
  • not working conversion of polynomials in one variable

I have no idea how to ask Gap for the Ring containing a polynomial..


New commits:

46e6f88first tentative to convert back polynomials from gap

comment:4 in reply to: ↑ 3 Changed 3 years ago by dimpase

Replying to chapoton:

I have no idea how to ask Gap for the Ring containing a polynomial..

if f is your polynomial, then

gap> Ring(f);

does what you ask.

comment:5 Changed 3 years ago by embray

  • Milestone changed from sage-7.3 to sage-wishlist

comment:6 Changed 3 years ago by chapoton

Hmm. Not very clear.. Does not look like a polynomial ring..

sage: x=polygen(QQ,'x')
sage: f=x+6
sage: g=libgap(f)
sage: g.Ring()
<ring with 1 generators>
sage: g.Ring().IsPolynomialRing()
false

comment:7 Changed 3 years ago by git

  • Commit changed from 46e6f88e14b7d3816d7aad194e83d26d80910e94 to 347ffdfd4819942c7a2e7338f611c626c52bd61f

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

347ffdfworking conversion back from libgap for univariate polynomials

comment:8 Changed 3 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Status changed from new to needs_review

Here is something that seems to work, even if somewhat ugly.

comment:9 Changed 3 years ago by chapoton

  • Milestone changed from sage-wishlist to sage-8.7

comment:10 Changed 3 years ago by chapoton

  • Dependencies #18266 deleted
  • Keywords libgap added

comment:11 Changed 3 years ago by chapoton

bot is morally green, please review

comment:12 Changed 3 years ago by tscrim

Dima, do you have any other comments? I am ready to set this to a positive review.

comment:13 Changed 3 years ago by dimpase

Can we get doctests for multivariate polynomials? And for different base rings?

comment:14 follow-up: Changed 3 years ago by chapoton

This will not work for multi-variate polynomials so far.

comment:15 Changed 3 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

1) I think should also be treated at the same time

  • (univariate) rational functions
  • (univariate) laurent polynomials

2) Instead of going through the coercion system in Sage you should just query gap base ring (function CoefficientsRing).

comment:16 follow-up: Changed 3 years ago by chapoton

By all ways, please do that! I have zero knowledge of gap, essentially.

comment:17 in reply to: ↑ 14 Changed 3 years ago by dimpase

Replying to chapoton:

This will not work for multi-variate polynomials so far.

There is code in this branch added for multivariate polynomial rings, so it was quite unclear why no examples for treating multivariate polynomials...

comment:18 in reply to: ↑ 16 Changed 3 years ago by vdelecroix

Replying to chapoton:

By all ways, please do that! I have zero knowledge of gap, essentially.

I am working on a commit.

comment:19 follow-up: Changed 3 years ago by vdelecroix

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Vincent Delecroix
  • Branch changed from u/chapoton/21020 to public/21020
  • Commit changed from 347ffdfd4819942c7a2e7338f611c626c52bd61f to ceeca172a50672c9335ebea90cb38376a55975d4
  • Status changed from needs_work to needs_review

Contrarily to what I thought, it seems not possible to extract the common ring of the coefficients from GAP. I kept the "common_parent".

Now works with (univriate) Laurent polynomials and rational functions.

Needs review again.


New commits:

ceeca1721020: univariate laurent polynomial, rational fraction

comment:20 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by dimpase

Replying to vdelecroix:

Contrarily to what I thought, it seems not possible to extract the common ring of the coefficients from GAP. I kept the "common_parent".

Did you try CoefficientRing(Ring(f)) for a GAP polynomial f?

(I'll have a look later today...)

comment:21 in reply to: ↑ 20 Changed 3 years ago by vdelecroix

Replying to dimpase:

Replying to vdelecroix:

Contrarily to what I thought, it seems not possible to extract the common ring of the coefficients from GAP. I kept the "common_parent".

Did you try CoefficientRing(Ring(f)) for a GAP polynomial f?

(I'll have a look later today...)

complete failure

sage: x = libgap.eval('Indeterminate(Integers, "x")')
sage: libgap.Ring(x)
<ring with 1 generators>
sage: libgap.CoefficientsRing(libgap.Ring(x))
Traceback (most recent call last):
...
AttributeError: No such attribute: CoefficientsRing.
Last edited 3 years ago by vdelecroix (previous) (diff)

comment:22 Changed 3 years ago by dimpase

If this works in GAP then it is matter of putting CoefficientRing into an appropriate list of GAP functions, or just calling libgap.function_factory(CoefficientRing)

comment:23 Changed 3 years ago by chapoton

Dima, do you have a concrete proposal ? Otherwise, one could start with the current thing and enhance that later in other tickets ?

What is the correct argument for CoefficientsRing? ?

sage: x=polygen(QQ,'x')
sage: X=libgap(x)
sage: R=X.Ring()
sage: R.CoefficientsRing()
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-9-003be256b736> in <module>()
----> 1 R.CoefficientsRing()

/home/chapoton/sage/local/lib/python2.7/site-packages/sage/libs/gap/element.pyx in sage.libs.gap.element.GapElement_MethodProxy.__call__ (build/cythonized/sage/libs/gap/element.c:18831)()
   2465             return GapElement_Function.__call__(self, * ([self.first_argument] + list(args)))
   2466         else:
-> 2467             return GapElement_Function.__call__(self, self.first_argument)
   2468 
   2469 

/home/chapoton/sage/local/lib/python2.7/site-packages/sage/libs/gap/element.pyx in sage.libs.gap.element.GapElement_Function.__call__ (build/cythonized/sage/libs/gap/element.c:18288)()
   2353             return make_any_gap_element(self.parent(), result)
   2354         except RuntimeError as msg:
-> 2355             raise ValueError('libGAP: ' + str(msg))
   2356         finally:
   2357             GAP_Leave()

ValueError: libGAP: Error, no method found! Error, no 1st choice method found for `CoefficientsRing' on 1 arguments

comment:24 Changed 3 years ago by dimpase

This looks like a GAP bug to me. I've opened #3159 for GAP.

Indeed, this should not stop us from going ahead with this ticket in some form.

comment:25 follow-up: Changed 3 years ago by chapoton

  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Frédéric Chapoton

Then I am satisfied with the current state, and ready to giev a positive review.

Vincent, d'accord ?

Version 0, edited 3 years ago by chapoton (next)

comment:26 in reply to: ↑ 25 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Replying to chapoton:

Then I am satisfied with the current state, and ready to give a positive review.

Vincent, d'accord ?

C'est bon pour moi. Merci Frédéric.

comment:27 Changed 3 years ago by fbissey

I don't know if it is only me but this ticket breaks building in sage-on-gentoo with cython-0.29.2

-> First build of interpreters
running build_cython
Enabling Cython debugging support
Updating Cython code....
warning: sage/graphs/mcqd.pyx:52:19: local variable 'clique_number' referenced before assignment
warning: sage/graphs/mcqd.pyx:55:57: local variable 'clique_number' referenced before assignment

Error compiling Cython file:
------------------------------------------------------------
...
from sage.structure.parent import Parent
from sage.rings.all import ZZ, QQ, RDF

from sage.groups.perm_gps.permgroup_element cimport PermutationGroupElement
from sage.combinat.permutation import Permutation
from sage.structure.element cimport coercion_model as cm
^
------------------------------------------------------------

sage/libs/gap/element.pyx:34:0: 'sage/structure/element/coercion_model.pxd' not found
warning: sage/libs/gap/element.pyx:238:21: local variable 'result' referenced before assignment
warning: sage/libs/gap/element.pyx:239:15: local variable 'result' referenced before assignment

Error compiling Cython file:
------------------------------------------------------------
...
            var = var.sage()
            num, den, val = self.CoefficientsOfUnivariateRationalFunction()
            num = num.sage()
            den = den.sage()
            val = val.sage()
            base_ring = cm.common_parent(*(num + den))
                         ^
------------------------------------------------------------

sage/libs/gap/element.pyx:1277:26: cimported module has no attribute 'common_parent'

Error compiling Cython file:
------------------------------------------------------------
...
            var = var.sage()
            num, den, val = self.CoefficientsOfUnivariateRationalFunction()
            num = num.sage()
            den = den.sage()
            val = val.sage()
            base_ring = cm.common_parent(*(num + den))
                         ^
------------------------------------------------------------

sage/libs/gap/element.pyx:1277:26: Compiler crash in AnalyseExpressionsTransform

ModuleNode.body = StatListNode(element.pyx:19:0)
StatListNode.stats[13] = CClassDefNode(element.pyx:369:5,
    as_name = u'GapElement',
    class_name = u'GapElement',
    doc = u"\n    Wrapper for all Gap objects.\n\n    .. NOTE::\n\n        In order to create ``GapElements`` you should use the\n        ``libgap`` instance (the parent of all Gap elements) to\n        convert things into ``GapElement``. You must not create\n        ``GapElement`` instances manually.\n\n    EXAMPLES::\n\n        sage: libgap(0)\n        0\n\n    If Gap finds an error while evaluating, a corresponding assertion is raised::\n\n        sage: libgap.eval('1/0')\n        Traceback (most recent call last):\n        ...\n        ValueError: libGAP: Error, Rational operations: <divisor> must not be zero\n\n    Also, a ``ValueError`` is raised if the input is not a simple expression::\n\n        sage: libgap.eval('1; 2; 3')\n        Traceback (most recent call last):\n        ...\n        ValueError: can only evaluate a single statement\n    ")
CClassDefNode.body = StatListNode(element.pyx:370:4)
StatListNode.stats[30] = DefNode(element.pyx:1202:4,
    doc = u'\n        Return the Sage equivalent of the :class:`GapElement`\n\n        EXAMPLES::\n\n            sage: libgap(1).sage()\n            1\n            sage: type(_)\n            <type \'sage.rings.integer.Integer\'>\n\n            sage: libgap(3/7).sage()\n            3/7\n            sage: type(_)\n            <type \'sage.rings.rational.Rational\'>\n\n            sage: libgap.eval(\'5 + 7*E(3)\').sage()\n            7*zeta3 + 5\n\n            sage: libgap(Infinity).sage()\n            +Infinity\n            sage: libgap(-Infinity).sage()\n            -Infinity\n\n            sage: libgap(True).sage()\n            True\n            sage: libgap(False).sage()\n            False\n            sage: type(_)\n            <... \'bool\'>\n\n            sage: libgap(\'this is a string\').sage()\n            \'this is a string\'\n            sage: type(_)\n            <... \'str\'>\n\n            sage: x = libgap.eval(\'Indeterminate(Integers, "x")\')\n\n            sage: p = x^2 - 2*x + 3\n            sage: p.sage()\n            x^2 - 2*x + 3\n            sage: p.sage().parent()\n            Univariate Polynomial Ring in x over Integer Ring\n\n            sage: p = x^-2 + 3*x\n            sage: p.sage()\n            x^-2 + 3*x\n            sage: p.sage().parent()\n            Univariate Laurent Polynomial Ring in x over Integer Ring\n\n            sage: p = (3 * x^2 + x) / (x^2 - 2)\n            sage: p.sage()\n            (3*x^2 + x)/(x^2 - 2)\n            sage: p.sage().parent()\n            Fraction Field of Univariate Polynomial Ring in x over Integer Ring\n        ',
    modifiers = [...]/0,
    name = u'sage',
    np_args_idx = [...]/0,
    num_required_args = 1,
    outer_attrs = [...]/2,
    py_wrapper_required = True,
    used = True)
File 'ExprNodes.py', line 5338, in infer_type: GeneralCallNode(element.pyx:1277:40,
    result_is_used = True,
    use_managed_ref = True)
File 'ExprNodes.py', line 6826, in infer_type: AttributeNode(element.pyx:1277:26,
    attribute = u'common_parent',
    is_attribute = 1,
    needs_none_check = True,
    result_is_used = True,
    use_managed_ref = True)

Compiler crash traceback from this point on:
  File "/usr/lib64/python2.7/site-packages/Cython/Compiler/ExprNodes.py", line 6826, in infer_type
    return node.entry.type
AttributeError: 'NoneType' object has no attribute 'type'
Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/Cython/Build/Dependencies.py", line 1244, in cythonize_one_helper
    return cythonize_one(*m)
  File "/usr/lib64/python2.7/site-packages/Cython/Build/Dependencies.py", line 1220, in cythonize_one
    raise CompileError(None, pyx_file)
CompileError: sage/libs/gap/element.pyx
************************************************************************
Traceback (most recent call last):
  File "setup.py", line 954, in <module>
    ext_modules = ext_modules)
  File "/usr/lib64/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/usr/lib64/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/usr/lib64/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "setup.py", line 857, in run
    build.run(self)
  File "/usr/lib64/python2.7/distutils/command/build.py", line 127, in run
    self.run_command(cmd_name)
  File "/usr/lib64/python2.7/distutils/cmd.py", line 326, in run_command
    self.distribution.run_command(command)
  File "/usr/lib64/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "setup.py", line 376, in run
    cache=False,
  File "/usr/lib64/python2.7/site-packages/Cython/Build/Dependencies.py", line 1088, in cythonize
    result.get(99999)  # seconds
  File "/usr/lib64/python2.7/multiprocessing/pool.py", line 572, in get
    raise self._value
CompileError: sage/libs/gap/element.pyx
************************************************************************
Error building the Sage library
************************************************************************

Going from from sage.structure.element cimport coercion_model as cm to from sage.structure.coerce cimport coercion_model as cm seem to fix the breakage.

comment:28 follow-up: Changed 3 years ago by chapoton

coercion_model has been moved by ticket #27021

so there is a conflict between here and there..

comment:29 in reply to: ↑ 28 Changed 3 years ago by fbissey

Replying to chapoton:

coercion_model has been moved by ticket #27021

so there is a conflict between here and there..

So that won't just be me, Volker is just trying to merge both tickets at the same time.

comment:30 Changed 3 years ago by chapoton

  • Dependencies set to #27021
  • Status changed from positive_review to needs_work

let us step back here then

comment:31 Changed 3 years ago by git

  • Commit changed from ceeca172a50672c9335ebea90cb38376a55975d4 to 0f2a7cca0bde11d1966341057d5ef815c15db844

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

44b4e79trac #27066 deprecate 5 things from the global namespace
184cef9Merge branch 'u/chapoton/27066' in 8.6
431b46ctrac 27066 fixing the failing doctests
70a2c6eadding some doctests, more to do
b1a19fbMerge branch 'public/21020' in 8.6
0f2a7cctrac 21020 fix import w.r.t. to 27021

comment:32 Changed 3 years ago by chapoton

  • Branch changed from public/21020 to u/chapoton/21020
  • Commit changed from 0f2a7cca0bde11d1966341057d5ef815c15db844 to 9437416a316059cf109207397aa35ebeb63f7f6c
  • Status changed from needs_work to needs_review

now the correct fix..


New commits:

9437416first tentative to convert back polynomials from gap

comment:33 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:34 Changed 3 years ago by tscrim

  • Reviewers changed from Vincent Delecroix, Frédéric Chapoton to Vincent Delecroix, Frédéric Chapoton, Travis Scrimshaw

comment:35 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/21020 to 9437416a316059cf109207397aa35ebeb63f7f6c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:36 Changed 22 months ago by dimpase

  • Commit 9437416a316059cf109207397aa35ebeb63f7f6c deleted

follow up (a bug): #30496

Note: See TracTickets for help on using tickets.