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

Fix crash and remove pexpect-Maxima usage in Y(m,n) #20939

Closed
rwst opened this issue Jul 5, 2016 · 46 comments
Closed

Fix crash and remove pexpect-Maxima usage in Y(m,n) #20939

rwst opened this issue Jul 5, 2016 · 46 comments

Comments

@rwst
Copy link

rwst commented Jul 5, 2016

sage: spherical_harmonic(3, 2, 1, 2*pi/3)
...
TypeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined.

The eval function of SphericalHarmonic uses pexpect-Maxima. Since this needs only a formula the call to Maxima could be replaced by a few lines of Python.

http://dlmf.nist.gov/14.30.E1

Replacing pexpect-Maxima is part of #17753.

Depends on #21614
Depends on #21963

Upstream: Fixed upstream, in a later stable release.

Component: symbolics

Author: Ralf Stephan

Branch/Commit: bb4630f

Reviewer: Travis Scrimshaw

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

@rwst rwst added this to the sage-7.3 milestone Jul 5, 2016
@rwst
Copy link
Author

rwst commented Aug 4, 2016

Dependencies: 16813

@rwst
Copy link
Author

rwst commented Aug 4, 2016

comment:1

This depends on the associated Legendre polynomials being implemented, e,g, #16813.

@rwst
Copy link
Author

rwst commented Aug 4, 2016

Changed dependencies from 16813 to #16813

@rwst
Copy link
Author

rwst commented Aug 4, 2016

comment:3

It won't be dependent on P(l,m,x) if the P special value formula is directly used in _eval_. The Y floating point evaluation is done via mpmath anyway.

@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Aug 4, 2016

Changed dependencies from #16813 to none

@rwst rwst added t: bug and removed t: enhancement labels Aug 4, 2016
@rwst rwst changed the title Remove pexpect-Maxima usage in Y(m,n) Fix crash and remove pexpect-Maxima usage in Y(m,n) Aug 4, 2016
@rwst
Copy link
Author

rwst commented Aug 4, 2016

@rwst
Copy link
Author

rwst commented Aug 4, 2016

Commit: f233afa

@rwst
Copy link
Author

rwst commented Aug 4, 2016

New commits:

f233afa20939: Fix crash and remove pexpect-Maxima usage in Y(m,n)

@rwst
Copy link
Author

rwst commented Aug 4, 2016

Author: Ralf Stephan

@rwst rwst modified the milestones: sage-7.3, sage-7.4 Aug 4, 2016
@tscrim
Copy link
Collaborator

tscrim commented Aug 4, 2016

comment:7

LGTM modulo

         Check that :trac:`20939` is fixed::
 
             sage: spherical_harmonic(3,2,1,2*pi/3)
             1/240*sqrt(30)*(-15*I*sqrt(7)*sqrt(3) -
-            ...15*sqrt(7))*cos(1)*sin(1)^2/sqrt(pi)
+            ... + 15*sqrt(7))*cos(1)*sin(1)^2/sqrt(pi)
         """
         if n in ZZ and m in ZZ and n > -1:
             if abs(m) > n:
                 return ZZ(0)
-            if (m == 0 and theta.is_zero()):
+            if m == 0 and theta.is_zero():
                 return sqrt((2*n+1)/4/pi)

@tscrim
Copy link
Collaborator

tscrim commented Aug 4, 2016

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

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

1a5cf6b20939: cosmetics

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

Changed commit from f233afa to 1a5cf6b

@rwst
Copy link
Author

rwst commented Aug 5, 2016

comment:9

That doesn't work as given. First the plus is a minus, and then the space after the ellipsis will make it fail too.

I'll assume the last commit is still what you wanted and set positive. Just revert if not. Thanks for the review.

@tscrim
Copy link
Collaborator

tscrim commented Aug 5, 2016

Changed commit from 1a5cf6b to a080e1e

@tscrim
Copy link
Collaborator

tscrim commented Aug 5, 2016

comment:10

So the ... is actually unneeded; I was thinking there was more than 2 terms. Please check my changes.


New commits:

a080e1eRemoving uncessary ... in doctest.

@tscrim
Copy link
Collaborator

tscrim commented Aug 5, 2016

@tscrim
Copy link
Collaborator

tscrim commented Nov 14, 2016

comment:23

Would it be possible to have a test to make sure this stays fixed?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 14, 2016

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

04f79cd20939: make sure atan2(0,0) returns always NaN

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 14, 2016

Changed commit from 31e6c64 to 04f79cd

@rwst
Copy link
Author

rwst commented Nov 14, 2016

comment:25

This commit contains a test for the specific NaN evaluation problem causing the second crash (the fix for the first crash is tested with #21614). The commit also fixes an inconsistency. As to a general test of the second problem I opened pynac/pynac#211

@tscrim
Copy link
Collaborator

tscrim commented Nov 14, 2016

comment:26

Thanks. Now back to the buildbots.

@kiwifb
Copy link
Member

kiwifb commented Nov 16, 2016

comment:27

I don know what Volker will report if anything but now I have a segfault in constant.py

sage: sig_on_count() # check sig_on/off pairings (virtual doctest) ## line 688 ##
0
sage: loads(dumps(NaN)) ## line 704 ##
------------------------------------------------------------------------
/usr/lib64/python2.7/site-packages/cysignals/signals.so(+0x4556)[0x7f4f180eb556]
/usr/lib64/python2.7/site-packages/cysignals/signals.so(+0x4a6c)[0x7f4f180eba6c]
/usr/lib64/python2.7/site-packages/cysignals/signals.so(+0x6d6f)[0x7f4f180edd6f]
/lib64/libpthread.so.0(+0x10d70)[0x7f4f1ce53d70]
/usr/lib64/python2.7/site-packages/sage/libs/pynac/pynac.so(+0x2940d)[0x7f4d3ece640d]
/usr/lib64/libpynac.so.8(_ZN5GiNaC8constant9unarchiveERKNS_12archive_nodeERNS_9containerINSt7__cxx114listEEE+0x196)[0x7f4d3e955dcc]
/usr/lib64/libpynac.so.8(_ZNK5GiNaC12archive_node9unarchiveERNS_9containerINSt7__cxx114listEEE+0xf4)[0x7f4d3e936836]
/usr/lib64/libpynac.so.8(_ZNK5GiNaC7archive12unarchive_exERKNS_9containerINSt7__cxx114listEEEj+0x130)[0x7f4d3e936a42]
/usr/lib64/python2.7/site-packages/sage/symbolic/expression.so(+0xec87f)[0x7f4d3e63787f]
/usr/lib64/python2.7/site-packages/sage/symbolic/expression.so(+0xecc2a)[0x7f4d3e637c2a]
/usr/lib64/libpython2.7.so.1.0(PyCFunction_Call+0xf1)[0x7f4f1d0ddbd9]
/usr/lib64/libpython2.7.so.1.0(PyObject_Call+0x5c)[0x7f4f1d0a8123]

@rwst
Copy link
Author

rwst commented Nov 16, 2016

comment:28

Confirmed. I'm sorry. With Pynac changes I have a testing procedure in place here, but with Python I was relying on patchbot. This issue is also somewhat unusual as to the surprising problems presented.

@vbraun
Copy link
Member

vbraun commented Nov 16, 2016

comment:29

Same here...

@kiwifb
Copy link
Member

kiwifb commented Nov 16, 2016

comment:30

Positive stuff for me, it made me realise that some bits of cysignals are not currently working properly on Gentoo - even so the tests pass :(

@rwst
Copy link
Author

rwst commented Nov 16, 2016

Upstream: Fixed upstream, in a later stable release.

@rwst
Copy link
Author

rwst commented Nov 16, 2016

Changed dependencies from #21614 to #21614, pynac-0.7.1

@rwst
Copy link
Author

rwst commented Nov 24, 2016

Changed dependencies from #21614, pynac-0.7.1 to #21614, #21963

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2016

Changed commit from 04f79cd to bb4630f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2016

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

bb4630fMerge branch 'develop' into t/20939/remove_pexpect_maxima_in_ymn-20939

@tscrim
Copy link
Collaborator

tscrim commented Dec 20, 2016

comment:34

Let's try again.

@vbraun
Copy link
Member

vbraun commented Jan 18, 2017

Changed branch from u/rws/remove_pexpect_maxima_in_ymn-20939 to bb4630f

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

4 participants