Shadowing Fricas function names leads to results un-backtranslatable to Sage.

[EmmanuelCharpentier]

Motivation : this ask.sagemath question.

Contemplate :

sage: var('x d b c a D C A B')
(x, d, b, c, a, D, C, A, B)
sage: foo = (b*x + a)^2*(D*x^3 + C*x^2 + B*x + A)/(d*x + c)
sage: integrate(foo, x, algorithm="fricas")
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

Bandwidth savings : Snip...

TypeError: unsupported operand parent(s) for *: 'Integer Ring' and '<class 'function'>'

But :

sage: var("capitalD")
capitalD
sage: integrate(foo.subs({D:capitalD}), x, algorithm="fricas").subs({capitalD:D})
1/60*(12*D*b^2*d^5*x^5 - 15*(D*b^2*c*d^4 - (2*D*a*b + C*b^2)*d^5)*x^4 + 20*(D*b^2*c^2*d^3 + (D*a^2 + 2*C*a*b + B*b^2)*d^5 - (2*D*a*b*c + C*b^2*c)*d^4)*x^3 - 30*(D*b^2*c^3*d^2 - (C*a^2 + 2*B*a*b + A*b^2)*d^5 + (D*a^2*c + (2*C*a*b + B*b^2)*c)*d^4 - (2*D*a*b*c^2 + C*b^2*c^2)*d^3)*x^2 + 60*(D*b^2*c^4*d - (C*a^2 + 2*B*a*b + A*b^2)*c*d^4 + (B*a^2 + 2*A*a*b)*d^5 + (D*a^2*c^2 + (2*C*a*b + B*b^2)*c^2)*d^3 - (2*D*a*b*c^3 + C*b^2*c^3)*d^2)*x - 60*(D*b^2*c^5 - A*a^2*d^5 - (C*a^2 + 2*B*a*b + A*b^2)*c^2*d^3 + (B*a^2 + 2*A*a*b)*c*d^4 + (D*a^2*c^3 + (2*C*a*b + B*b^2)*c^3)*d^2 - (2*D*a*b*c^4 + C*b^2*c^4)*d)*log(d*x + c))/d^6

The cherry on the pie :

sage: bool(integrate(foo.subs({D:capitalD}), x, algorithm="fricas").diff(x).subs({capitalD:D})==foo)
True

The use of the variable D shadows the Fricas function D, which is not a problem for Fricas, but is not known to the fricas._sage_() method, which uses hardcoded equivalences for categorizing syntactic trees' atoms.

CC: @mwhansen @mantepse @slel @fchapoton @tscrim

Component: interfaces

Keywords: symbolic fricas interface

Author: Martin Rubey

Branch/Commit: 52099de

Reviewer: Travis Scrimshaw

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

[mwageringel]

comment:1

There are similar problems with other interfaces as well, such as Giac #30133.

[mantepse]

comment:2

I think the problem is here:

    @staticmethod
    def _parse_other(s, start=0, make_fun=False):
        """
...
        This function uses the symbol table to translate symbols
        which are not function calls.  At least ``%pi`` is an
        example showing that this may be necessary::

            sage: FriCASElement._parse_other("%pi")
            (pi, 2)

        """
        a = start
        b = len(s)
        while a < b and s[a] not in FriCASElement._WHITESPACE and s[a] != FriCASElement._RIGHTBRACKET:
            a += 1

        e = s[start:a]
        if make_fun:
            try:
                e = symbol_table["fricas"][e]
            except KeyError:
                e = function(e)
        else:
            try:
                e = ZZ(e)
            except TypeError:
                try:
                    e = float(e)
                except ValueError:
                    try:
                       e = symbol_table["fricas"][e]
                    except KeyError:
                        e = var(e.replace("%", "_"))
        return e, a - 1

In line -4, we check whether e is in the symbol table, because it might be a constant like %pi or %i or infinity. I guess the easiest way out is to check for these explicitely. I am not sure whether we should remove constants from the symbol table entirely, but it may be less confusing to do so.

[mantepse]

Branch: u/mantepse/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_

[mantepse]

Commit: ba08317

[mantepse]

comment:4

It might make sense to move the imports and the register_symbol statements from _sage_expression and _parse_other to module level, but I don't know what's best practice for that.

---

New commits:

ba08317

separate constants and functions in _parse_other

[mantepse]

Author: Martin Rubey

[slel]

comment:6

Changing cc list, guessing the intention was to cc @mwhansen, who was involved
in past FriCAS-related tickets such as #6318, #6517, #9258, #9354, rather than @sagetrac-dmhansen, who contributed Weil pairings in #4964.

[sagetrac-git]

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

ad9ea7a

Merge branch 'develop' of git://trac.sagemath.org/sage into t/31849/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_

[sagetrac-git]

Changed commit from ba08317 to ad9ea7a

[sagetrac-git]

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

6b75c5d

update docstring

[sagetrac-git]

Changed commit from ad9ea7a to 6b75c5d

[sagetrac-git]

Changed commit from 6b75c5d to 03a3469

[sagetrac-git]

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

03a3469

Merge branch 'develop' of git://trac.sagemath.org/sage into t/31849/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_

[sagetrac-git]

Changed commit from 03a3469 to 577e476

[sagetrac-git]

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

577e476

improve docstring

[mantepse]

comment:11

Emmanuel, could you have a look, please?

[mantepse]

comment:13

Frederic, do you have a minute for this one?

[mantepse]

comment:14

Travis, could you have a quick look, please? I think it would be good to have this in the next release.

[tscrim]

comment:15

Do we always want to continually recreate the constants dict? Wouldn't it make more sense to be created at the module level?

Also, could you revert this change as the line ends after """ is Sage's convention:

diff --git a/src/sage/interfaces/fricas.py b/src/sage/interfaces/fricas.py
index a73330e..faac5e2 100644
--- a/src/sage/interfaces/fricas.py
+++ b/src/sage/interfaces/fricas.py
@@ -1281,8 +1281,7 @@ class FriCASElement(ExpectElement):
 
     @staticmethod
     def _parse_other(s, start=0, make_fun=False):
-        """
-        Parse the initial part of a string, assuming that it is an
+        """Parse the initial part of a string, assuming that it is an
         atom, but not a string.
 
         Symbols and numbers must not contain ``FriCASElement._WHITESPACE`` and

[mantepse]

comment:16

Would the following at module level be OK? (It works, but I am not sure about coding style.)

There is also a warning: "resolving lazy import infinity during startup"

lazy_import('sage.symbolic.constants', ["I", "e", "pi"])
lazy_import('sage.rings.infinity', ["infinity"])

FRICAS_CONSTANTS = {'%i': I,
                    '%e': e,
                    '%pi': pi,
                    'infinity': infinity,
                    'plusInfinity': infinity,
                    'minusInfinity': -infinity}

[tscrim]

comment:17

Yes, that would be good. Although you don't need to lazily import infinity here because it is already imported globally at startup.

[sagetrac-git]

Changed commit from 577e476 to 52099de

[sagetrac-git]

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

52099de

move dictionary with constants to module level

[mantepse]

comment:19

Done! (I would have liked to do the same with the long sequence of register_symbol statements in _sage_expression. This failed because of a circular dependency caused by register_symbol(pi, {'fricas': 'pi'}). Possibly I could move them into a class method, which I call from FriCAS.__init__?

[tscrim]

comment:20

Thank you.

I don't immediately know what would cause that circular import. Of course, the ideal thing would be to remove the circular import, but if that is not feasible, your solution of doing something in the __init__ is much better than doing this in something that is repeatedly called.

Are you going to do that on this ticket or a separate one?

[tscrim]

Reviewer: Travis Scrimshaw

[mantepse]

comment:21

I think it is better to do this in a separate ticket, because doing so I discovered a bug in the rootOf translation.

I opened #32133 for the translation, and I'll do the move there as a first step.

[tscrim]

comment:22

Okay, positive review.

[vbraun]

Changed branch from u/mantepse/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_ to 52099de