Opened 4 months ago

Closed 2 months ago

#31849 closed defect (fixed)

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

Reported by: charpent Owned by:
Priority: major Milestone: sage-9.4
Component: interfaces Keywords: symbolic fricas interface
Cc: mhansen, mantepse, slelievre, chapoton, tscrim Merged in:
Authors: Martin Rubey Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 52099de (Commits, GitHub, GitLab) Commit: 52099de1de29a1c21e4a4d80af3f4a1a9cab1954
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (23)

comment:1 Changed 4 months ago by gh-mwageringel

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

comment:2 Changed 4 months ago by mantepse

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.

comment:3 Changed 4 months ago by mantepse

  • Branch set to u/mantepse/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_

comment:4 Changed 4 months ago by mantepse

  • Commit set to ba083177a1b5a767ff6d61ab939f466425922453

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:

ba08317separate constants and functions in _parse_other

comment:5 Changed 4 months ago by mantepse

  • Authors set to Martin Rubey
  • Status changed from new to needs_review

comment:6 Changed 4 months ago by slelievre

  • Cc mhansen slelievre added; dmhansen removed

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

comment:7 Changed 3 months ago by git

  • Commit changed from ba083177a1b5a767ff6d61ab939f466425922453 to ad9ea7a054fedadd61649e81f8c9586f1f5307b4

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

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

comment:8 Changed 3 months ago by git

  • Commit changed from ad9ea7a054fedadd61649e81f8c9586f1f5307b4 to 6b75c5dc2628082cf18b87d08ee5ad51d2c15b5d

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

6b75c5dupdate docstring

comment:9 Changed 3 months ago by git

  • Commit changed from 6b75c5dc2628082cf18b87d08ee5ad51d2c15b5d to 03a3469c53c577ef523673f84dd08baa343ba524

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

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

comment:10 Changed 3 months ago by git

  • Commit changed from 03a3469c53c577ef523673f84dd08baa343ba524 to 577e47634c4e0f4e978c919c20a9986ebdc9ad34

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

577e476improve docstring

comment:11 Changed 3 months ago by mantepse

Emmanuel, could you have a look, please?

comment:12 Changed 3 months ago by mantepse

  • Cc chapoton added

comment:13 Changed 3 months ago by mantepse

Frederic, do you have a minute for this one?

comment:14 Changed 3 months ago by mantepse

  • Cc tscrim added

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

comment:15 Changed 3 months ago by tscrim

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:

  • src/sage/interfaces/fricas.py

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

comment:16 Changed 3 months ago by mantepse

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}

comment:17 Changed 3 months ago by tscrim

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

comment:18 Changed 3 months ago by git

  • Commit changed from 577e47634c4e0f4e978c919c20a9986ebdc9ad34 to 52099de1de29a1c21e4a4d80af3f4a1a9cab1954

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

52099demove dictionary with constants to module level

comment:19 Changed 3 months ago by mantepse

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__?

comment:20 Changed 3 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

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?

comment:21 Changed 2 months ago by mantepse

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.

comment:22 Changed 2 months ago by tscrim

  • Status changed from needs_review to positive_review

Okay, positive review.

comment:23 Changed 2 months ago by vbraun

  • Branch changed from u/mantepse/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_ to 52099de1de29a1c21e4a4d80af3f4a1a9cab1954
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.