Opened 7 years ago

Last modified 5 years ago

#16587 closed defect

f(expr).n() fails for all generalized functions — at Version 19

Reported by: rws Owned by:
Priority: major Milestone: sage-7.3
Component: symbolics Keywords: sgn, evaluation
Cc: kcrisman Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/f_expr__n___fails_for_all_generalized_functions (Commits, GitHub, GitLab) Commit: cc22a9b489a841b23a6e9455bfea80a0b423c0e1
Dependencies: #17130, #17285 Stopgaps:

Status badges

Description (last modified by rws)

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

UPDATE: The problem also appears with polylog(2.,.9).n() for example.

Change History (19)

comment:1 Changed 7 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 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:3 Changed 7 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-up: Changed 7 years ago by jdemeyer

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

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

Thanks. This begs for a subclass of BuiltinFunction.

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

Replying to rws:

Thanks. This begs for a subclass of BuiltinFunction.

I don't see why.

comment:7 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 years ago by jdemeyer

Related: #17285

comment:14 Changed 7 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 7 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 7 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 7 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 7 years ago by rws

  • Description modified (diff)
Note: See TracTickets for help on using tickets.