Regression: Many mathematica doctests now fail

[sagetrac-flawrence]

Since #3587, which implements a sage() method for mathematica elements, many mathematica doctests fail. E.g:

sage: def math_bessel_K(nu,x): 
     ...       return mathematica(nu).BesselK(x).N(20).sage() 
     ... 
     sage: math_bessel_K(2,I) 
NotImplementedError: Unable to parse 
Mathematica output: 
-2.5928861754911969781676606702635284285719718407749199115289`20.1494653502 82203 
+ 
0.1804899720669620266296208808560650432663536549483055754141`18.99213497581 376*i 

apply only attachment: trac_8495-rewrite-sage.2.patch

CC: @burcin @jasongrout

Component: interfaces

Author: Felix Lawrence

Reviewer: Mike Hansen, Burcin Erocal, David Kirkby

Merged: sage-4.7.alpha3

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

[sagetrac-flawrence]

comment:1

Before #3587, mathematica's output was sent to ExpectElement._sage_repr() in expect.py, which called repr() and tidied up the results (converting {} to [], stripping new line characters etc) then sage_eval() was called on the results. With this approach, mathematica functions that returned numbers, symbolic variables or arrays could be imported successfully into sage. The approach had the disadvantage that all symbolic variables had to be passed in manually as locals, and that functions couldn't be translated from mathematica to their equivalents in sage.

#3587 instead calls str() on mathematica's results (which has the alarming option ascii_art = True), then replaces []s by (), changes everything to lower case and sends it to SR. This works for simple functions but fails for arrays and probably anything affected by ascii_art = True

I'm not familiar with SR, but at a minimum MathematicaElement.sage() should be patched to call sage_repr() instead of str(). I don't know whether SR has all the functionality of sage_eval(), e.g. supporting arrays.

[sagetrac-flawrence]

comment:2

I've uploaded a patch that has a thorough rewrite of MathematicaElement.sage() to get the functionality from #3587 while keeping the functionality from before it (lists, complex numbers, numbers in scientific notation...). I still need to write some documentation for the top of the file (i.e. documentation that makes it into the reference manual) but before I do that and submit this for formal review I'd like wise comments about my approach, e.g. "The way you convert function names is really inefficient and problematic, do it this way...", or "You can efficiently get a list of all sage functions recognised by sage_eval() by ...".

Also if someone could check the doctests on a 32-bit computer and let me know the result that they get instead of
[[1.00000000000000, 4], pi, 3.20000000000000*e100, I]
that would be grand.

[sagetrac-flawrence]

comment:3

I've updated the patch file so that it updates the Mathematica module documentation. This needs to be doctested on a 32-bit computer at some point.

[sagetrac-flawrence]

Author: flawrence

[burcin]

Changed author from flawrence to Felix Lawrence

[mwhansen]

Reviewer: Mike Hansen

[mwhansen]

comment:5

There is dictionary in place from Mathematica to Sage.

sage: sage.symbolic.pynac.symbol_table['mathematica']
{'Log[2]': log2, 'Cos': cos, 'DiracDelta': dirac_delta, 'EulerGamma': euler_gamma, 'Glaisher': glaisher, 'Sqrt': <function sqrt at 0x2c20f50>, 'Factorial': factorial, 'Khinchin': khinchin, 'Catalan': catalan, '(1+Sqrt[5])/2': golden_ratio, 'Binomial': binomial, 'PolyGamma': psi, 'HeavisideTheta': heaviside, 'KroneckerDelta': kronecker_delta, 'Pi': pi, 'UnitStep': unit_step, 'Sin': sin, 'Gamma': gamma, 'Sign': sgn}

This is constructed at runtime and is filled in by the __init__ methods of the functions and constants.

[sagetrac-whuss]

comment:6

Replying to @sagetrac-flawrence:

I've uploaded a patch that has a thorough rewrite of MathematicaElement.sage() to get the functionality from #3587 while keeping the functionality from before it (lists, complex numbers, numbers in scientific notation...). I still need to write some documentation for the top of the file (i.e. documentation that makes it into the reference manual) but before I do that and submit this for formal review I'd like wise comments about my approach, e.g. "The way you convert function names is really inefficient and problematic, do it this way...", or "You can efficiently get a list of all sage functions recognised by sage_eval() by ...".

Also if someone could check the doctests on a 32-bit computer and let me know the result that they get instead of
[[1.00000000000000, 4], pi, 3.20000000000000*e100, I]
that would be grand.

On 32-bit Debian I get the same output. There is only one doctest failure:

./sage -t  -only-optional=mathematica "devel/sage/sage/interfaces/mathematica.py"
sage -t -only-optional=mathematica "devel/sage/sage/interfaces/mathematica.py"
**********************************************************************
File "./sage-4.4.4/devel/sage/sage/interfaces/mathematica.py", line 281:
    sage: math_bessel_K(2,I)                      # optional - mathematica
Expected:
    0.180489972066962*I - 2.592886175491197
Got:
    -2.59288617549119697816765132253822887 + 0.180489972066962026629620880838378650*I
**********************************************************************

But this is probably unrelated to this patch, since also without this patch applied I get
things like:

sage: mathematica('N[Pi, 1]')
3.1415926535897932385
sage: mathematica('N[Pi, 10]')
3.1415926535897932385
sage: mathematica('N[Pi, 11]')
3.1415926535897932384626433836

[sagetrac-flawrence]

comment:7

Replying to @mwhansen:

There is dictionary in place from Mathematica to Sage.

sage: sage.symbolic.pynac.symbol_table['mathematica']
{'Log[2]': log2, 'Cos': cos, 'DiracDelta': dirac_delta, 'EulerGamma': euler_gamma, 'Glaisher': glaisher, 'Sqrt': <function sqrt at 0x2c20f50>, 'Factorial': factorial, 'Khinchin': khinchin, 'Catalan': catalan, '(1+Sqrt[5])/2': golden_ratio, 'Binomial': binomial, 'PolyGamma': psi, 'HeavisideTheta': heaviside, 'KroneckerDelta': kronecker_delta, 'Pi': pi, 'UnitStep': unit_step, 'Sin': sin, 'Gamma': gamma, 'Sign': sgn}

Excellent! I've updated the patch so that it uses that dictionary. I also added the ability to pass a locals dictionary to _sage_, which complements and/or overrides the symbol_table['mathematica'] dictionary.

The documentation for this function won't be very visible, since it starts with an underscore, so I was contemplating setting up a mathematica-specific .sage function that accepts a locals dictionary and has a copy of this documentation. In the end I didn't do this because I don't understand the consequences (or why there are separate _sage_ and sage functions at all). Opinions?

Replying to @sagetrac-whuss:

On 32-bit Debian I get the same output. There is only one doctest failure:

But this is probably unrelated to this patch, since also without this patch applied I get things like:

Yes, I'm not sure it's related to the patch either. For the record I get the correct behaviour with the patch:

sage: mathematica('N[Pi, 1]')
3.1
sage: mathematica('N[Pi, 10]')
3.1415926536
sage: mathematica('N[Pi, 11]')
3.14159265359

[burcin]

comment:9

This looks great! Many thanks for your work Felix.

Here is my review:

• Can you change the line lengths to be less than 78 characters? Some of us still edit code using the terminal.
• There should be an empty line after :: in the documentation. You can also put the :: sign right after the text, you don't need to make it a separate line.
• On line 648 of interfaces/mathematica.py, you can change the test if result.find('"') != -1: to if '"' in result:
• The dictionary merge in the _sage_() method might be expensive. I think it's better to do it only if the locals dictionary is not empty. Perhaps lsymbols = symbol_table['mathematica'].copy(); lsymbols.update(locals) would be faster.
• Using sage_eval() has lot's of security implications. Can we replace it with sage.calculus.calculus.SR_parser or sage.calculus.calculus.symbolic_expression_from_string() in this case?

This is definitely an improvement over what we have currently. I'd really like to see it included in the next release.

[burcin]

Changed reviewer from Mike Hansen to Mike Hansen, Burcin Erocal

[burcin]

comment:10

One more:

• The last two hunks applied to sage/symbolic/constants.py should be removed. The conversions dictionary defines how the current constant is represented in the given system. The values stored in the dictionary should be in MMA syntax.

[sagetrac-flawrence]

comment:11

Thanks for your suggestions Burcin. I implemented all of them, and made some significant further changes:

Now we actually check whether each function exists, before we try to convert the entire expression. This allows us to check several variations of mma's function names, such as a downcased version, a down_cased version, and the original name. In order to do this I needed to add an option to sage.calculus.calculus._find_func() whereby it wouldn't automatically create a symbolic function if none were present in Sage. We now also try the same downcasing tricks for constants.

In order to convert from CamelCase to camel_case I used code from stack overflow. I presume such code is public domain and doesn't require attribution in the docstring.

I also realised that the documentation for _sage_ won't get into Sage's HTML documentation so I added a short section in the module's documentation that reproduces some of the material in the docstring for sage().

[sagetrac-flawrence]

Attachment: diff_with_previous_review.patch.gz

What's changed since burcin's review

[sagetrac-drkirkby]

comment:12

What patch(s) is supposed to be applied here? trac_8495-rewrite-_sage_.patch applied cleanly to sage-4.6.2.rc1, but diff_with_previous_review.patch neither applies cleanly on its own, or after applying trac_8495-rewrite-_sage_.patch.

[burcin]

comment:17

Replying to @sagetrac-flawrence:

BTW, the use of sage.calculus.calculus.symbolic_expression_from_string seems to limit the accuracy of results:

> sage: from sage.calculus.calculus import symbolic_expression_from_string as sefs 
> sage: repr(mathematica(pi/2).N(50))
> '1.57079632679489661923132169163975144209858469968755'
> sage: sefs('1.57079632679489661923132169163975144209858469968755')
> 1.57079632679
> sage: sage_eval('1.57079632679489661923132169163975144209858469968755')
> 1.5707963267948966192313216916397514420985846996875

sage_eval gets it right, but Burcin noted above that using it is a security risk. I guess my suggestion would be to stick with symbolic_expression_from_string and open a ticket on improving its accuracy. Thoughts?

Sounds good to me. Can you open that ticket? :)

[sagetrac-drkirkby]

comment:18

Replying to @sagetrac-flawrence:

OK, that's quite strange. On my OS X 10.6 64-bit box with Sage 4.6.1, all tests pass. But the two doctests that failed on your computer don't even call .sage(), which is the main thing that this ticket modifies!

Em, it is strange.

I'm a bit tied up now, but will repeat later.

I'm actually willing to give this a positive review, on the basis that it fixes a very large number of the problems, and no new bugs are introduced. But I'll run the tests later and see what can be done.

Dave

[sagetrac-flawrence]

comment:19

Replying to @burcin:

Replying to @sagetrac-flawrence:

sage_eval gets it right, but Burcin noted above that using it is a security risk. I guess my suggestion would be to stick with symbolic_expression_from_string and open a ticket on improving its accuracy. Thoughts?

Sounds good to me. Can you open that ticket? :)

Done: #10898

[sagetrac-flawrence]

comment:20

Hi Dave,

Have you had a chance to investigate what was happening on Solaris?

Cheers,
Felix

[sagetrac-drkirkby]

comment:21

Replying to @sagetrac-flawrence:

Hi Dave,

Have you had a chance to investigate what was happening on Solaris?

Cheers,
Felix

Can you clarify the exact commands you want tested with and without the patch?

[sagetrac-flawrence]

comment:22

ARGH! I upgraded to 4.6.2 (from 4.6.1), ran the tests and get the same errors as Dave.

#9032 modified Sage's .n(), and added (in two files)

n = numerical_approx
N = n

This means that Mathematica's N[] is no longer being called - instead Sage's numerical_approx() is being called, and it can't handle certain mma objects. Furthermore the argument taken by numerical_approx is the number of bits (I think?) whereas N[50] gives 50 sig figs.

I think that the way to fix this is by adjusting precedences so that on mathematica objects, mathematica functions take priority. I also think that fixing this is beyond the scope of this ticket (which was a rewrite of ._sage_). I have created a new ticket regarding this issue: #10968

[sagetrac-drkirkby]

comment:23

Replying to @sagetrac-flawrence:

ARGH! I upgraded to 4.6.2 (from 4.6.1), ran the tests and get the same errors as Dave.

I'm glad I;m not going mad.

Since the changes reduces the number of failures from 17 to 2, I believe this should be merged, so positive review. The remaining issues can be sorted out on #10968

[sagetrac-drkirkby]

Changed reviewer from Mike Hansen, Burcin Erocal to Mike Hansen, Burcin Erocal, David Kirkby

[jdemeyer]

comment:24

Problems while building the documentation:

dochtml.log:/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python2.6/site-packages/sage/interfaces/mathematica.py:docstring of sage.interfaces.mathematica:282: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
dochtml.log:/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python2.6/site-packages/sage/interfaces/mathematica.py:docstring of sage.interfaces.mathematica:284: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
dochtml.log:/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/local/lib/python2.6/site-packages/sage/interfaces/mathematica.py:docstring of sage.interfaces.mathematica:277: (ERROR/3) Unexpected indentation.

[sagetrac-flawrence]

Attachment: trac_8495-rewrite-sage.patch.gz

[sagetrac-flawrence]

comment:25

Attachment: trac_8495-rewrite-sage.2.patch.gz

I made some minor formatting changes to the documentation to avoid the above problems. The documentation now builds without errors and looks OK on my machine.

I forgot to tick the 'replace attachment' box; please disregard the older (non ".2.") patch.

[sagetrac-drkirkby]

comment:26

Yes, the documentation builds ok for me. That was something I did not check before I must admit.

Dave

[jdemeyer]

Merged: sage-4.7.alpha3