f(expr,hold).n() fails for all generalized functions #16587

rwst · 2014-06-29T16:16:48Z

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)).

Depends on #17130
Depends on #17285

CC: @kcrisman

Component: symbolics

Keywords: sgn, evaluation

Author: Ralf Stephan

Branch/Commit: e7880b7

Reviewer: Paul Masson

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

The text was updated successfully, but these errors were encountered:

kcrisman · 2014-06-30T13:00:19Z

comment:1

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

rwst · 2014-11-02T16:56:08Z

comment:3

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

jdemeyer · 2014-11-03T09:39:57Z

comment:4

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

rwst · 2014-11-03T17:05:36Z

comment:5

Thanks. This begs for a subclass of BuiltinFunction.

jdemeyer · 2014-11-03T19:59:43Z

comment:6

Replying to @rwst:

Thanks. This begs for a subclass of BuiltinFunction.

I don't see why.

jdemeyer · 2014-11-03T20:00:53Z

comment:7

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).

rwst · 2014-11-04T09:54:28Z

Branch: u/rws/f_expr__n___fails_for_all_generalized_functions

rwst · 2014-11-04T09:56:47Z

Commit: 44ca3ad

rwst · 2014-11-04T09:56:47Z

comment:9

Replying to @jdemeyer:

Replying to @rwst:

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 remote-tracking 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`

jdemeyer · 2014-11-04T10:20:08Z

comment:10

Replying to @rwst:

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.

jdemeyer · 2014-11-04T10:26:45Z

comment:11

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...

jdemeyer · 2014-11-04T10:32:39Z

comment:12

We also should decide whether

sage: sgn(cos(3/2))

should automatically simplify to

or remain unevaluated.

jdemeyer · 2014-11-04T10:37:27Z

comment:13

Related: #17285

sagetrac-git · 2014-11-04T16:06:26Z

Changed commit from 44ca3ad to cc22a9b

sagetrac-git · 2014-11-04T16:06:26Z

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

`cc22a9b`	`16587: roll back previous commit, less the small fixes`

rwst · 2014-11-04T16:09:24Z

comment:15

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.

jdemeyer · 2014-11-05T20:30:44Z

Dependencies: #17130, #17285

jdemeyer · 2014-11-05T20:30:44Z

comment:16

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.

rwst · 2015-03-31T06:04:32Z

comment:17

Is this related?
https://groups.google.com/forum/?hl=en#!topic/sage-devel/rkkdK5qbPY0

rwst · 2015-03-31T07:27:54Z

comment:18

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().

rwst · 2015-07-08T13:03:11Z

Changed branch from u/rws/f_expr__n___fails_for_all_generalized_functions to u/rws/16587

rwst · 2015-07-08T13:06:04Z

Changed commit from cc22a9b to 7ef7c92

rwst · 2015-07-08T13:06:04Z

comment:22

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:

`7ef7c92`	`16587: f(expr,hold).n() fails for all generalized functions`

rwst · 2015-07-08T13:06:04Z

Author: Ralf Stephan

paulmasson · 2016-07-07T01:39:23Z

comment:23

I tried to checkout this ticket and rebuild Sage but that failed. Branch needs updating.

paulmasson · 2016-07-07T01:39:23Z

Reviewer: Paul Masson

sagetrac-git · 2016-07-07T05:43:31Z

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

`e7159eb`	`Merge branch 'develop' into t/16587/16587`

sagetrac-git · 2016-07-07T05:43:31Z

Changed commit from 7ef7c92 to e7159eb

rwst · 2016-07-07T05:46:14Z

comment:25

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.

paulmasson · 2016-07-08T01:31:34Z

comment:26

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?

rwst · 2016-07-08T13:12:48Z

comment:27

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.

rwst · 2016-07-08T14:28:50Z

Changed branch from u/rws/16587 to u/rws/16587-1

rwst · 2016-07-08T14:31:58Z

comment:29

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:

`e7880b7`	`16587: f(expr,hold).n() fails for all generalized functions`

rwst · 2016-07-08T14:31:58Z

Changed commit from e7159eb to e7880b7

paulmasson · 2016-07-08T21:47:29Z

comment:30

Doctests pass. Functions behaved as expected as per ticket. Ready to go.

vbraun · 2016-07-09T16:29:25Z

Changed branch from u/rws/16587-1 to e7880b7

rwst added this to the sage-6.3 milestone Jun 29, 2014

rwst added c: symbolics labels Jun 29, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014

rwst changed the title ~~evaluation of sgn(expr) fails~~ f(expr).n() fails for all generalized functions Nov 2, 2014

This comment has been minimized.

Sign in to view

rwst changed the title ~~f(expr).n() fails for all generalized functions~~ f(expr,hold).n() fails for all generalized functions Jul 6, 2015

This comment has been minimized.

Sign in to view

rwst modified the milestones: sage-6.4, sage-6.8 Jul 8, 2015

rwst added the s: needs review label Jul 8, 2015

paulmasson mannequin modified the milestones: sage-6.8, sage-7.3 Jul 7, 2016

paulmasson mannequin added s: positive review and removed s: needs review labels Jul 8, 2016

vbraun removed the s: positive review label Jul 9, 2016

vbraun closed this as completed in 70e22c7 Jul 9, 2016

jdemeyer mentioned this issue Dec 2, 2014

Fix coercion bugs in symbolic functions #17130

Closed

f(expr,hold).n() fails for all generalized functions #16587

f(expr,hold).n() fails for all generalized functions #16587

Comments

rwst commented Jun 29, 2014

kcrisman commented Jun 30, 2014

rwst commented Nov 2, 2014

jdemeyer commented Nov 3, 2014

rwst commented Nov 3, 2014

jdemeyer commented Nov 3, 2014

jdemeyer commented Nov 3, 2014

rwst commented Nov 4, 2014

rwst commented Nov 4, 2014

rwst commented Nov 4, 2014

jdemeyer commented Nov 4, 2014

jdemeyer commented Nov 4, 2014

jdemeyer commented Nov 4, 2014

jdemeyer commented Nov 4, 2014

sagetrac-git mannequin commented Nov 4, 2014

sagetrac-git mannequin commented Nov 4, 2014

rwst commented Nov 4, 2014

jdemeyer commented Nov 5, 2014

jdemeyer commented Nov 5, 2014

rwst commented Mar 31, 2015

rwst commented Mar 31, 2015

This comment has been minimized.

This comment has been minimized.

rwst commented Jul 8, 2015

This comment has been minimized.

rwst commented Jul 8, 2015

rwst commented Jul 8, 2015

rwst commented Jul 8, 2015

paulmasson mannequin commented Jul 7, 2016

paulmasson mannequin commented Jul 7, 2016

sagetrac-git mannequin commented Jul 7, 2016

sagetrac-git mannequin commented Jul 7, 2016

rwst commented Jul 7, 2016

paulmasson mannequin commented Jul 8, 2016

rwst commented Jul 8, 2016

rwst commented Jul 8, 2016

rwst commented Jul 8, 2016

rwst commented Jul 8, 2016

paulmasson mannequin commented Jul 8, 2016

vbraun commented Jul 9, 2016