Opened 15 months ago
Closed 13 months ago
#31849 closed defect (fixed)
Shadowing Fricas function names leads to results unbacktranslatable to Sage.
Reported by:  charpent  Owned by:  

Priority:  major  Milestone:  sage9.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: 
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 15 months ago by
comment:2 Changed 15 months ago by
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 15 months ago by
 Branch set to u/mantepse/shadowing_fricas_function_names_leads_to_results_un_backtranslatable_to_sage_
comment:4 Changed 15 months ago by
 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:
ba08317  separate constants and functions in _parse_other

comment:5 Changed 15 months ago by
 Status changed from new to needs_review
comment:6 Changed 15 months ago by
 Cc mhansen slelievre added; dmhansen removed
comment:7 Changed 14 months ago by
 Commit changed from ba083177a1b5a767ff6d61ab939f466425922453 to ad9ea7a054fedadd61649e81f8c9586f1f5307b4
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_

comment:8 Changed 14 months ago by
 Commit changed from ad9ea7a054fedadd61649e81f8c9586f1f5307b4 to 6b75c5dc2628082cf18b87d08ee5ad51d2c15b5d
Branch pushed to git repo; I updated commit sha1. New commits:
6b75c5d  update docstring

comment:9 Changed 14 months ago by
 Commit changed from 6b75c5dc2628082cf18b87d08ee5ad51d2c15b5d to 03a3469c53c577ef523673f84dd08baa343ba524
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_

comment:10 Changed 14 months ago by
 Commit changed from 03a3469c53c577ef523673f84dd08baa343ba524 to 577e47634c4e0f4e978c919c20a9986ebdc9ad34
Branch pushed to git repo; I updated commit sha1. New commits:
577e476  improve docstring

comment:11 Changed 14 months ago by
Emmanuel, could you have a look, please?
comment:12 Changed 14 months ago by
 Cc chapoton added
comment:13 Changed 14 months ago by
Frederic, do you have a minute for this one?
comment:14 Changed 13 months ago by
 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 13 months ago by
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): 1281 1281 1282 1282 @staticmethod 1283 1283 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 1286 1285 atom, but not a string. 1287 1286 1288 1287 Symbols and numbers must not contain ``FriCASElement._WHITESPACE`` and
comment:16 Changed 13 months ago by
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 13 months ago by
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 13 months ago by
 Commit changed from 577e47634c4e0f4e978c919c20a9986ebdc9ad34 to 52099de1de29a1c21e4a4d80af3f4a1a9cab1954
Branch pushed to git repo; I updated commit sha1. New commits:
52099de  move dictionary with constants to module level

comment:19 Changed 13 months ago by
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 13 months ago by
 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 13 months ago by
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 13 months ago by
 Status changed from needs_review to positive_review
Okay, positive review.
comment:23 Changed 13 months ago by
 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
There are similar problems with other interfaces as well, such as Giac #30133.