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: sage-7.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 rws)

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/problem-with-sign-sgn-and-n/ by Louis Cypher:

sage: M = sgn(cos(3/2))
sage: M.n()
TypeError                                 Traceback (most recent call last)
<ipython-input-5-0f11e9bd1e87> in <module>()
----> 1 M.n()

/home/ralf/sage/local/lib/python2.7/site-packages/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 kcrisman

  • Cc kcrisman added

You can always feel free to cc: me on any ticket coming from something I was involved in :)

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:3 Changed 5 years ago by rws

  • 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)
<ipython-input-18-b9940d9ad473> in <module>()
----> 1 M = sgn((Integer(3)/Integer(2)),hold=True); M.n()

/home/ralf/sage/local/lib/python2.7/site-packages/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 follow-ups: Changed 5 years ago by jdemeyer

Isn't the problem simply that these functions are missing an _evalf_() method?

comment:5 follow-up: Changed 5 years ago by rws

Thanks. This begs for a subclass of BuiltinFunction.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by jdemeyer

Replying to rws:

Thanks. This begs for a subclass of BuiltinFunction.

I don't see why.

comment:7 Changed 5 years ago by jdemeyer

Let me also mention that this ticket touches on similar issues as #17130. So if you make a patch, I would prefer it to be based on #17130 (although that ticket is still in needs_review).

comment:8 Changed 5 years ago by rws

  • Branch set to u/rws/f_expr__n___fails_for_all_generalized_functions

comment:9 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by rws

  • 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:

6d10729Simplify numerical evaluation of BuiltinFunctions
b6e1ed4Merge remote-tracking branches 'origin/u/jdemeyer/ticket/17131' and 'origin/u/jdemeyer/ticket/17133' into ticket/17130
b83feedMerge branch 'u/jdemeyer/ticket/17130' of trac.sagemath.org:sage into t/16587/f_expr__n___fails_for_all_generalized_functions
44ca3ad16587: factor out general evalf

comment:10 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

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 jdemeyer

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 jdemeyer

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 jdemeyer

Related: #17285

comment:14 Changed 5 years ago by git

  • Commit changed from 44ca3ad0846f9e991e1cc54db796db304efc6938 to cc22a9b489a841b23a6e9455bfea80a0b423c0e1

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

cc22a9b16587: roll back previous commit, less the small fixes

comment:15 Changed 5 years ago by rws

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 jdemeyer

  • 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:18 in reply to: ↑ 4 Changed 5 years ago by rws

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 rws

  • Description modified (diff)

comment:20 Changed 4 years ago by rws

  • 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 rws

  • 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 rws

  • Authors set to Ralf Stephan
  • Commit changed from cc22a9b489a841b23a6e9455bfea80a0b423c0e1 to 7ef7c92cba5293f3d2c9f84e8488164b390b2458
  • Description modified (diff)
  • Milestone changed from sage-6.4 to sage-6.8
  • Status changed from new to needs_review

Replying to jdemeyer:

Isn't the problem simply that these functions are missing an _evalf_() method?

You were of course right. Please review. The polylog issue mentioned previously is part of #18386.


New commits:

7ef7c9216587: f(expr,hold).n() fails for all generalized functions

comment:23 Changed 3 years ago by paulmasson

  • Milestone changed from sage-6.8 to sage-7.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 git

  • Commit changed from 7ef7c92cba5293f3d2c9f84e8488164b390b2458 to e7159eb400a9ddf09b36c7a7c2d4b42be24df3bd

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

e7159ebMerge branch 'develop' into t/16587/16587

comment:25 Changed 3 years ago by rws

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 follow-up: Changed 3 years ago by paulmasson

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)
<ipython-input-27-089ce283036d> 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 rws

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 up-to-date 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 rws

  • Branch changed from u/rws/16587 to u/rws/16587-1

comment:29 Changed 3 years ago by rws

  • Commit changed from e7159eb400a9ddf09b36c7a7c2d4b42be24df3bd to e7880b7caa636ff4d24bccacb5c21f4285d1164c

I rewrote it all and pushed a new branch (this is also a way to get rid of merge commits). There was danger of infinite loops (see #12121). It also resolved the kronecker issue. Please review.


New commits:

e7880b716587: f(expr,hold).n() fails for all generalized functions

comment:30 Changed 3 years ago by paulmasson

  • 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 vbraun

  • Branch changed from u/rws/16587-1 to e7880b7caa636ff4d24bccacb5c21f4285d1164c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.