Opened 5 years ago
Closed 3 years ago
#16587 closed defect (fixed)
f(expr,hold).n() fails for all generalized functions
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  symbolics  Keywords:  sgn, evaluation 
Cc:  kcrisman  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Paul Masson 
Report Upstream:  N/A  Work issues:  
Branch:  e7880b7 (Commits)  Commit:  e7880b7caa636ff4d24bccacb5c21f4285d1164c 
Dependencies:  #17130, #17285  Stopgaps: 
Description (last modified by )
sage: M = sgn((3/2),hold=True); M.n() ... TypeError: cannot evaluate symbolic expression numerically
The original problem is now resolved, it was reported in http://ask.sagemath.org/question/8535/problemwithsignsgnandn/ by Louis Cypher:
sage: M = sgn(cos(3/2)) sage: M.n() TypeError Traceback (most recent call last) <ipythoninput50f11e9bd1e87> in <module>() > 1 M.n() /home/ralf/sage/local/lib/python2.7/sitepackages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._numerical_approx (build/cythonized/sage/symbolic/expression.cpp:24086)() TypeError: cannot evaluate symbolic expression numerically
kcrisman:
Problem seems to be that in M.n??
we see that it's looking for is_a_numeric(x._gobj)
but apparently that fails, as does the constant, so it thinks we are looking at evaluating sgn(cos(x))
instead of sgn(cos(3/2))
.
Change History (31)
comment:1 Changed 5 years ago by
 Cc kcrisman added
comment:2 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:3 Changed 5 years ago by
 Summary changed from evaluation of sgn(expr) fails to f(expr).n() fails for all generalized functions
Actually all functions in generalized.py
show this problem. Moreover, the cos
can be left out:
sage: M = sgn((3/2),hold=True); M.n()  TypeError Traceback (most recent call last) <ipythoninput18b9940d9ad473> in <module>() > 1 M = sgn((Integer(3)/Integer(2)),hold=True); M.n() /home/ralf/sage/local/lib/python2.7/sitepackages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._numerical_approx (build/cythonized/sage/symbolic/expression.cpp:27093)() TypeError: cannot evaluate symbolic expression numerically
All the generalized functions have this code in _eval_()
and I believe the bug to be there:
try: approx_x = ComplexIntervalField()(x) if bool(approx_x.imag() == 0): # x is real if bool(approx_x.real() == 0): # x is zero return ... else: return ... else: return ... # x is complex except Exception: # x is symbolic pass return None
comment:4 followups: ↓ 18 ↓ 22 Changed 5 years ago by
Isn't the problem simply that these functions are missing an _evalf_()
method?
comment:5 followup: ↓ 6 Changed 5 years ago by
Thanks. This begs for a subclass of BuiltinFunction
.
comment:6 in reply to: ↑ 5 ; followup: ↓ 9 Changed 5 years ago by
comment:7 Changed 5 years ago by
comment:8 Changed 5 years ago by
 Branch set to u/rws/f_expr__n___fails_for_all_generalized_functions
comment:9 in reply to: ↑ 6 ; followup: ↓ 10 Changed 5 years ago by
 Commit set to 44ca3ad0846f9e991e1cc54db796db304efc6938
Replying to jdemeyer:
Replying to rws:
Thanks. This begs for a subclass of
BuiltinFunction
.I don't see why.
Because I didn't want to create an evalf
for everyone of these. Documentation writing is pending your approval of this construct.
New commits:
6d10729  Simplify numerical evaluation of BuiltinFunctions

b6e1ed4  Merge remotetracking branches 'origin/u/jdemeyer/ticket/17131' and 'origin/u/jdemeyer/ticket/17133' into ticket/17130

b83feed  Merge branch 'u/jdemeyer/ticket/17130' of trac.sagemath.org:sage into t/16587/f_expr__n___fails_for_all_generalized_functions

44ca3ad  16587: factor out general evalf

comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to rws:
Because I didn't want to create an
evalf
for everyone of these.
But now you create a gen_eval()
method for everyone of those, why is that better?
Documentation writing is pending your approval of this construct.
No, I still do not see the reason for a new class.
comment:11 Changed 5 years ago by
Also, do we really need the ComplexIntervalField
stuff in _eval_
? I would replace it by a simple if x == 0
or something like other functions...
comment:12 Changed 5 years ago by
We also should decide whether
sage: sgn(cos(3/2))
should automatically simplify to
1
or remain unevaluated.
comment:13 Changed 5 years ago by
Related: #17285
comment:14 Changed 5 years ago by
 Commit changed from 44ca3ad0846f9e991e1cc54db796db304efc6938 to cc22a9b489a841b23a6e9455bfea80a0b423c0e1
Branch pushed to git repo; I updated commit sha1. New commits:
cc22a9b  16587: roll back previous commit, less the small fixes

comment:15 Changed 5 years ago by
Please confirm this is what you think should go in as basic change to address the original issue. I feel sgn(cos(3/2))
should give 1
. I'll try now if this works without ComplexIntervalField
.
comment:16 Changed 5 years ago by
 Dependencies set to #17130, #17285
After thinking about this more, perhaps the ComplexIntervalField
solution isn't so bad after all. In any case, #17285 is the main reason that
sgn(cos(3/2))
doesn't currently simplify to 1
.
comment:17 Changed 5 years ago by
comment:18 in reply to: ↑ 4 Changed 5 years ago by
Replying to jdemeyer:
Isn't the problem simply that these functions are missing an
_evalf_()
method?
To finally answer this, no, it will still trigger the exception with M = sgn((3/2),hold=True); M.n()
and with M = sgn(cos(3/2)); M.n()
.
comment:19 Changed 5 years ago by
 Description modified (diff)
comment:20 Changed 4 years ago by
 Description modified (diff)
 Summary changed from f(expr).n() fails for all generalized functions to f(expr,hold).n() fails for all generalized functions
comment:21 Changed 4 years ago by
 Branch changed from u/rws/f_expr__n___fails_for_all_generalized_functions to u/rws/16587
comment:22 in reply to: ↑ 4 Changed 4 years ago by
 Commit changed from cc22a9b489a841b23a6e9455bfea80a0b423c0e1 to 7ef7c92cba5293f3d2c9f84e8488164b390b2458
 Description modified (diff)
 Milestone changed from sage6.4 to sage6.8
 Status changed from new to needs_review
comment:23 Changed 3 years ago by
 Milestone changed from sage6.8 to sage7.3
 Reviewers set to Paul Masson
I tried to checkout this ticket and rebuild Sage but that failed. Branch needs updating.
comment:24 Changed 3 years ago by
 Commit changed from 7ef7c92cba5293f3d2c9f84e8488164b390b2458 to e7159eb400a9ddf09b36c7a7c2d4b42be24df3bd
Branch pushed to git repo; I updated commit sha1. New commits:
e7159eb  Merge branch 'develop' into t/16587/16587

comment:25 Changed 3 years ago by
The branch merges cleanly with develop (green link) so it should have worked after git merge develop
on your machine. However, I've done it for you in the branch now, so you can simply check out.
comment:26 followup: ↓ 27 Changed 3 years ago by
Doctests pass. dirac_delta
, heaviside
, unit_step
and sign
function as expected for real arguments.
kronecker_delta
returns an unexpected error for real arguments:
sage: kronecker_delta(1,2,hold=True).n()  TypeError Traceback (most recent call last) <ipythoninput27089ce283036d> in <module>() > 1 kronecker_delta(Integer(1),Integer(2),hold=True).n() /Users/Masson/Downloads/GitHub/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._numerical_approx (/Users/Masson/Downloads/GitHub/sage/src/build/cythonized/sage/symbolic/expression.cpp:31167)() 5430 pass # try again with complex 5431 kwds['parent'] = R.complex_field() > 5432 x = x._convert(kwds) 5433 5434 # we have to consider constants as well, since infinity is a constant /Users/Masson/Downloads/GitHub/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._convert (/Users/Masson/Downloads/GitHub/sage/src/build/cythonized/sage/symbolic/expression.cpp:9677)() 1242 0.989992496600445*sqrt(2) 1243 """ > 1244 cdef GEx res = self._gobj.evalf(0, kwds) 1245 return new_Expression_from_GEx(self._parent, res) 1246 TypeError: _evalf_() takes exactly 2 arguments (5 given)
Looks like its trying to evaluate arguments as complex numbers.
And for future reference, is it expected practice for people to merge the current develop branch before pushing to Trac? Or is it a matter of how the branch is assembled, especially for older tickets, and I just need to pay attention to what gets pulled?
comment:27 in reply to: ↑ 26 Changed 3 years ago by
Replying to paulmasson:
And for future reference, is it expected practice for people to merge the current develop branch before pushing to Trac? Or is it a matter of how the branch is assembled, especially for older tickets, and I just need to pay attention to what gets pulled?
There should be as few merge commits as possible but you can't always adhere to that.
As author, you of course need to merge if what you uploaded before doesn't with develop (red link on ticket). But as you saw, there was a branch that was so old that it could no longer be compiled so I needed to merge it too. Note that as a reviewer you do not need to do git trac checkout 12345; git merge develop
to get an uptodate version of an older branch (that still merges cleanly). You can also branch from develop and pull changes: git checkout b tmp; git trac pull 12345
. This way your local Sage is not changed to the version of the branch and only the specific branch changes need to be compiled in.
So, to answer, in principle you need not merge the current develop branch before pushing as long as the branch merges cleanly with develop. A good reason to do it nevertheless would be that you want to make sure your changes pass tests (which are always done by patchbot or the RM with develop). What you want to pay attention to as reviewer OTOH is the age of the last merge commit (or the first commit if none) and rather pull (see above) than checkout if older than say 18 months.
comment:28 Changed 3 years ago by
 Branch changed from u/rws/16587 to u/rws/165871
comment:29 Changed 3 years ago by
 Commit changed from e7159eb400a9ddf09b36c7a7c2d4b42be24df3bd to e7880b7caa636ff4d24bccacb5c21f4285d1164c
comment:30 Changed 3 years ago by
 Status changed from needs_review to positive_review
Doctests pass. Functions behaved as expected as per ticket. Ready to go.
comment:31 Changed 3 years ago by
 Branch changed from u/rws/165871 to e7880b7caa636ff4d24bccacb5c21f4285d1164c
 Resolution set to fixed
 Status changed from positive_review to closed
You can always feel free to cc: me on any ticket coming from something I was involved in :)