Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: conversion of Mathematica's Sqrt to Sage fails #31756

Closed
mwageringel opened this issue Apr 29, 2021 · 18 comments
Closed

regression: conversion of Mathematica's Sqrt to Sage fails #31756

mwageringel opened this issue Apr 29, 2021 · 18 comments

Comments

@mwageringel
Copy link

This works with Sage 9.2, but fails with 9.3.rc4:

sage: mathematica('Sqrt[x]').sage()
Sqrt(x)

The result is an uppercase symbolic function, but should be the lowercase function sqrt in Sage.

A doctest in interfaces/mathematica.py now fails:

File "src/sage/interfaces/mathematica.py", line 776, in sage.interfaces.mathematica.MathematicaElement._sage_
Failed example:
    m.sage()                          # optional - mathematica
Expected:
    (cos(1/x) - 1)^2*sin(sqrt(-x^2 + 1))
Got:
    (cos(1/x) - 1)^2*sin(Sqrt(-x^2 + 1))

This is also reproducible with mathematica_free, see this post on sage-release.

This also affects the Gamma function:

File "src/sage/functions/gamma.py", line 731, in sage.functions.gamma._mathematica_gamma
Failed example:
    gamma(4/3)._mathematica_().sage()       # indirect doctest, optional - mathematica
Expected:
    gamma(4/3)
Got:
    Gamma(4/3)

CC: @seblabbe @EmmanuelCharpentier @nbruin @egourgoulhon @rwst @DaveWitteMorris

Component: interfaces

Keywords: mathematica

Author: Markus Wageringel

Branch/Commit: 6bacab2

Reviewer: Emmanuel Charpentier

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

@mwageringel mwageringel added this to the sage-9.3 milestone Apr 29, 2021
@mwageringel
Copy link
Author

comment:1

Bisecting leads to #31047, which was merged in 9.3.rc0.

@mwageringel

This comment has been minimized.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Apr 30, 2021

comment:4

Cc'd myself and the participants to #31047.

Another unrelated work (wrapping Sympy's symbolic logical functions) also showed the necessity of testing Maxima conversions bilaterally.

@spaghettisalat
Copy link

comment:5

This is caused by the mathematica symbol table containing entries (for 'Sqrt' and 'Gamma') which are python functions but not of class sage.symbolic.function.Function. The implementation of symbolic_expression_from_string in #31047 sorts the entries of the symbol table into vars and functions by checking for this class in order for the parser to be able to search the function and variable namespaces separately.

Unfortunately, it is not easily possible to extend the sorting procedure to sort raw python functions into the function namespace, because sagemath variables are also callable objects.

There are basically two ways to solve the problem:

  1. convert the entries in the symbol table to proper sagemath functions (this is what I did in Conversion of symbolic functions with latex_name or nargs from maxima and sympy is broken #31047 for the single sympy symbol table entry affected by this problem)
  2. change the parser to scan the whole symbol table. This is a bit problematic since it means that the parser will happily accept nonsensical input like pi(10).

@mwageringel
Copy link
Author

comment:6

Replying to @spaghettisalat:

This is caused by the mathematica symbol table containing entries (for 'Sqrt' and 'Gamma') which are python functions but not of class sage.symbolic.function.Function.

Thank you for the analysis. Indeed, the Gamma function is affected by this, as well. In the case of the Gamma function, I recall that this is intended: The function _mathematica_gamma in the symbol table is a wrapper around the gamma function in Sage, which is needed because the order or number of arguments of the Gamma function in Mathematica is different. While it may be possible to turn this into a symbolic Function, this would only be needed as a workaround for this bug.

It is also possible to register new symbols, for example to convert custom functions from Mathematica to equivalent definitions in Sage, which could be Python functions. This still seems to work though.

sage: from sage.libs.pynac.pynac import register_symbol
sage: foo = lambda x: x^2 + 1
sage: register_symbol(foo, dict(mathematica='Foo'))
sage: mathematica.eval('Foo[x_]:=x^2+1')
sage: Foo = mathematica('Foo')
sage: Foo.sage() is foo
True

Some of the functions in Sage's global namespace are also Python functions:

sage: type(sqrt), type(gamma), type(log)
(<class 'function'>, <class 'function'>, <class 'function'>)

Unfortunately, it is not easily possible to extend the sorting procedure to sort raw python functions into the function namespace, because sagemath variables are also callable objects.

How about checking whether the object is callable and not an instance of SageObject? Or whether it is an instance of function?

@mwageringel

This comment has been minimized.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 1, 2021

comment:8

I was on the same track as gh-spaghettisalat, but less advanced (I'm new at debugging Sage...).

gh-mwageringel' suggestion of making sqrt and gamma proper Sage functions seems reasonable : if that's the only "corner cases" encountered so far, and the need to create proper Sage function for further additions is well documented, this would be also an appreciable cleanup.

One might also plan to create proper Sage function wrappers around the current functions This might even be delegated to sage.symbolic.function_factory.function, if that's acceptable in Sage library code.

//Warning : diversion ahead...//

I would be very interested in testing this "lite" solution because, I plan to propose to wrap Sympy's symbolic logical functions And, Or and Not, which are sorely lacking in Sage :

The Python operators and, or and not work well according to a "programming) point of view, but spew nonsense when applied to mathematical predication : compare

sage: (pi>3) and (pi<5)
pi < 5

with

sage: from sympy import sympify, And
sage: And(*map(sympify, ((pi>3), (pi<5))))
(pi > 3) & (pi < 5)

Writing a symbolic wrapper with function isn't quite difficult, adding a _sage_ method to sympy's And is also easy ; the difficulty is to integrate this with Maxima :

sage: var("y")
y
sage: maxima("%pi>y and %pi<x")
%pi>yand%pi<x

which, of course, fails to backtranslate in Sage :

sage: maxima("%pi>y and %pi<x").sage()
---------------------------------------------------------------------------
SyntaxError                               Traceback (most recent call last)

[ Backtrace : snip... ]

TypeError: unable to make sense of Maxima expression 'pi>yandpi<x' in Sage

Any hint about this problem in the Maxima interface would be very welcome...

//End of diversion...//

So I'm interested in your comments on gh-mwageringel' suggestions.

@mwageringel
Copy link
Author

Commit: 6bacab2

@mwageringel
Copy link
Author

Branch: u/gh-mwageringel/31756

@mwageringel
Copy link
Author

Author: Markus Wageringel

@mwageringel
Copy link
Author

comment:9

Ok, here is a minimal fix that treats Python functions as functions as well. Please review.

I have not tested the optional internet doctests because of SSL problems on my machine, but I assume that the current branch also fixes the errors mentioned on sage-release.

(There is one unrelated but harmless Mathematica doctest failure in interfaces/mathematica.py, related to the recent html/latex changes, which I am going to fix in #29136.)


New commits:

6bacab231756: parse Python functions in expressions as functions instead of variables

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 2, 2021

comment:10

Replying to @mwageringel:

Replying to @spaghettisalat:

[ Snip... ]

How about checking whether the object is callable and not an instance of SageObject? Or whether it is an instance of function?

One concurring datapoint : without your patch, Python functions are ignored by Mathematia objects _sage_ method :

sage: # Attempt to define the (lower) incomplete beta function
sage: def foo(x, a, b): return x^a*gamma(a)*hypergeometric((a, -b + 1), (a + 1,), x)/gamma(a + 1)
sage: mathematica("Foo[x, a, b]")._sage_(locals={"Foo":foo})
Foo(x, a, b)

whereas Sage's symbolic functions are not :

sage: foo=function("foo", nargs=3, eval_func=lambda self, x, a, b: x^a*gamma(a)*hypergeometric((a, -b + 1), (a + 1,), x)/gamma(a + 1))
sage: mathematica("Foo[x, a, b]")._sage_(locals={"Foo":foo})
x^a*gamma(a)*hypergeometric((a, -b + 1), (a + 1,), x)/gamma(a + 1)

Testing your patch : stay tuned...

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 2, 2021

comment:11

After rebasing your branch on develop :

Replying to @EmmanuelCharpentier:

Replying to @mwageringel:

Replying to @spaghettisalat:

[ Snip... ]

A first cursory look is satisfying :

sage: mathematica.Sqrt(x^2)._sage_()
sqrt(x^2)
sage: with assuming(x>0): mathematica.Sqrt(x^2)._sage_().simplify()
x
sage: mathematica.Gamma(x)._sage_()
gamma(x)
sage: (mathematica.Gamma(x)/mathematica.Gamma(x-1)).sage().simplify_factorial()
x - 1
sage: def foo(x, a, b): return x^a*gamma(a)*hypergeometric((a, -b + 1), (a + 1,), x)/gamma(a + 1)
sage: mathematica("Foo[x, a, b]")._sage_(locals={"Foo":foo})
x^a*gamma(a)*hypergeometric((a, -b + 1), (a + 1,), x)/gamma(a + 1)

Similarly :

harpent@zen-book-flip:/usr/local/sage-9$ sage -t --long src/sage/interfaces/mathematica.py 
Running doctests with ID 2021-05-02-19-58-43-32f2badb.
Git branch: t/31756/31756
Using --optional=build,debian,dochtml,dot2tex,fricas,gap_jupyter,gap_packages,kenzo,libsemigroups,pip,pysingular,sage,sage_spkg,singular_jupyter
Doctesting 1 file.
sage -t --long --warn-long 237.9 --random-seed=0 src/sage/interfaces/mathematica.py
    [19 tests, 0.04 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.1 seconds
    cpu time: 0.0 seconds
    cumulative wall time: 0.0 seconds

Now running ptestlong, stay tuned...

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 2, 2021

comment:12

ptestlong gives me exactly the same results than those obtained with the {{{develop branch (one transient timeout related to Pari, twe permanent errors related to GAP).

==> positive review (as soon as my damn Internet connection gets up again...), notwithstanding the lint failure.

@mwageringel
Copy link
Author

comment:13

Thank you.

@mwageringel
Copy link
Author

Reviewer: Emmanuel Charpentier

@fchapoton
Copy link
Contributor

comment:17

milestone to 9.4, as 9.3 has been released

@fchapoton fchapoton modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@vbraun
Copy link
Member

vbraun commented May 27, 2021

Changed branch from u/gh-mwageringel/31756 to 6bacab2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants