Opened 11 years ago

Closed 9 years ago

Last modified 8 years ago

#12289 closed enhancement (fixed)

pass algorithm argument to custom numeric evaluation methods

Reported by: Burcin Erocal Owned by: Burcin Erocal
Priority: major Milestone: sage-6.2
Component: symbolics Keywords: pynac sd35.5 sd40.5 sd48
Cc: Benjamin Jones, D.S. McNeil, Karl-Dieter Crisman, Keshav Kini, Eviatar Bach, Volker Braun Merged in:
Authors: Burcin Erocal, Benjamin Jones Reviewers: Karl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: ef6d2de (Commits, GitHub, GitLab) Commit:
Dependencies: #13933, #4102, #15198 Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Custom numeric evaluation functions defined in the _evalf_() method of symbolic functions accept only the parent of the result as an argument. We should expand this to allow passing an algorithm parameter as well.

Merge together with dependencies (i.e. there are circular dependencies).

Attachments (10)

trac_12289-evalf_dictionary.patch (3.5 KB) - added by Burcin Erocal 11 years ago.
trac_12289-add_algorithm_arg.patch (9.0 KB) - added by Burcin Erocal 11 years ago.
trac_12289-example.patch (456 bytes) - added by Burcin Erocal 11 years ago.
trac_12289-evalf_dictionary_rebase.patch (4.2 KB) - added by Benjamin Jones 11 years ago.
rebase to sage-5.0 + pynac-0.2.4 spkg
trac_12289_py_float_fix.patch (683 bytes) - added by Benjamin Jones 11 years ago.
fix py_float delcaration
trac_12289-py_float-fix-rebase.patch (663 bytes) - added by Karl-Dieter Crisman 9 years ago.
trac_12289-add_algorithm-rebase.patch (8.3 KB) - added by Karl-Dieter Crisman 9 years ago.
trac_12289-more-alg.patch (5.1 KB) - added by Karl-Dieter Crisman 9 years ago.
trac_12289-evalf_dictionary_rebase.take2.patch (4.2 KB) - added by Burcin Erocal 9 years ago.
trac_12289-last_touches.patch (3.0 KB) - added by Burcin Erocal 9 years ago.

Download all attachments as: .zip

Change History (55)

Changed 11 years ago by Burcin Erocal

Changed 11 years ago by Burcin Erocal

Changed 11 years ago by Burcin Erocal

Attachment: trac_12289-example.patch added

comment:1 Changed 11 years ago by Burcin Erocal

Cc: Benjamin Jones added

The evalf_dict branch of Pynac available here, with the attached patches applied to the Sage library, allows this:

sage: cot(1/2).n(algorithm='foo')
11.0000000000000
sage: cot(1/2).n()
1.83048772171245

The docstrings for _convert() etc. methods of symbolic expressions need to be updated to reflect these changes.

comment:2 Changed 11 years ago by Benjamin Jones

Keywords: sd35.5 added

I'm doing some testing of these patches along with rebasing #1173. Thanks Burcin!

comment:3 Changed 11 years ago by Benjamin Jones

Cc: D.S. McNeil added
Keywords: sd40.5 added

comment:4 Changed 11 years ago by Benjamin Jones

I'm trying to rebase the patch trac_12289-evalf_dictionary.patch to sage-5.0, but I'm running into a problem with the type of py_funcs.py_float. In the patch py_float has type object (*)(object, object) but in sage-5.0, it has type object (*)(object, PyObject *) I guess because of a pynac change in the meantime.

To be more specific, with this type def for py_float:

cdef public object py_float(object n, object kwds) except +:

I rebuild sage and get:

Error compiling Cython file:
------------------------------------------------------------
...
    py_funcs.py_is_prime = &py_is_prime

    py_funcs.py_integer_from_long = &py_integer_from_long
    py_funcs.py_integer_from_python_obj = &py_integer_from_python_obj
    
    py_funcs.py_float = &py_float
                       ^
------------------------------------------------------------

sage/symbolic/pynac.pyx:2040:24: Cannot assign type 'object (*)(object, object)' to 'object (*)(object, PyObject *)'

Any suggestions (burcin?)

comment:5 Changed 11 years ago by Karl-Dieter Crisman

Cc: Karl-Dieter Crisman added

comment:6 Changed 11 years ago by Keshav Kini

Cc: Keshav Kini added

Changed 11 years ago by Benjamin Jones

rebase to sage-5.0 + pynac-0.2.4 spkg

Changed 11 years ago by Benjamin Jones

fix py_float delcaration

comment:7 Changed 11 years ago by Benjamin Jones

I've been working on this ticket today at sd40.5. I've got burcin's changes working in sage-5.0 with a caveat. Here's how to get the example in comment:1 working in sage-5.0:

  1. install the patched pynac-0.2.4.p1 spkg at http://sage.math.washington.edu/home/bjones/pynac-0.2.4.p1.spkg. this spkg is the pynac-0.2.4 spkg from #12950 which has been patched to include the changes from burcin's pynac-wip branch of pynac mentioned above.
  1. apply all patches at #12950, here is a shell script to help that (run from $SAGE_ROOT/devel/sage): http://sage.math.washington.edu/home/bjones/trac_12950_apply.sh
  1. apply the patches to $SAGE_ROOT/devel/sage
  1. sage -b

comment:8 Changed 11 years ago by Karl-Dieter Crisman

Description: modified (diff)

Patchbot, though, should only apply trac_12289-evalf_dictionary_rebase.patch, trac_12289-add_algorithm_arg.patch, and trac_12289_py_float_fix.patch

comment:9 Changed 11 years ago by D.S. McNeil

So that we don't forget to fix it later:

sage: erf(1).n(algorithm='foo')
0.842700792949715
sage: erf(0)
0
sage: parent(erf(0))
Integer Ring
sage: erf(0).n(algorithm='foo')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/Users/mcneil/sagedev/sage-5.1.beta0b/sage-5.1.beta0/devel/sage-hack2/sage/<ipython console> in <module>()

/Users/mcneil/sagedev/sage-5.1.beta0b/sage-5.1.beta0/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.numerical_approx (sage/structure/element.c:4988)()

TypeError: numerical_approx() got an unexpected keyword argument 'algorithm'

comment:10 in reply to:  7 Changed 10 years ago by Karl-Dieter Crisman

  1. install the patched pynac-0.2.4.p1 spkg at http://sage.math.washington.edu/home/bjones/pynac-0.2.4.p1.spkg. this spkg is the pynac-0.2.4 spkg from #12950 which has been patched to include the changes from burcin's pynac-wip branch of pynac mentioned above.

Is this stuff in the current Pynac 0.2.6?

  1. apply all patches at #12950, here is a shell script to help that (run from $SAGE_ROOT/devel/sage): http://sage.math.washington.edu/home/bjones/trac_12950_apply.sh

Can now be skipped, presumably.

  1. apply the patches to $SAGE_ROOT/devel/sage
  1. sage -b

comment:11 Changed 10 years ago by Eviatar Bach

Cc: Eviatar Bach added

comment:12 Changed 9 years ago by Karl-Dieter Crisman

Some of these patches have bitrotted slightly. Updates coming up.

comment:13 Changed 9 years ago by Karl-Dieter Crisman

Regarding Doug's comment on

erf(0).n(algorithm='foo')

Is this something we want to fix here? I'm not sure whether this is behavior that is supported with this patch, though perhaps it should be. I think that so far this is only putting in infrastructure, right? I don't see any doctests here, for instance.

Changed 9 years ago by Karl-Dieter Crisman

Changed 9 years ago by Karl-Dieter Crisman

comment:14 Changed 9 years ago by Karl-Dieter Crisman

Description: modified (diff)

comment:15 Changed 9 years ago by Karl-Dieter Crisman

Description: modified (diff)

Patchbot, apply trac_12289-evalf_dictionary_rebase.patch , trac_12289-add_algorithm-rebase.patch , and trac_12289-py_float-fix-rebase.patch

comment:16 Changed 9 years ago by Karl-Dieter Crisman

I think that we definitely need at least one example of how to do this if we aren't going to have any other documentation here.

comment:17 Changed 9 years ago by Karl-Dieter Crisman

Authors: Burcin ErocalBurcin Erocal, Benjamin Jones
Reviewers: Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones
Status: newneeds_review
Work issues: doctests, documentation

We need to fix this.

----------------------------------------------------------------------
sage -t sage/symbolic/expression.pyx  # 3 doctests failed
sage -t sage/functions/other.py  # Killed due to segmentation fault
sage -t sage/functions/exp_integral.py  # 25 doctests failed
sage -t sage/functions/log.py  # 4 doctests failed
sage -t sage/symbolic/pynac.pyx  # 2 doctests failed
sage -t sage/symbolic/function.pyx  # 1 doctest failed
----------------------------------------------------------------------

But it does work!

comment:18 Changed 9 years ago by Karl-Dieter Crisman

Status: needs_reviewneeds_work

comment:19 Changed 9 years ago by Eviatar Bach

Keywords: sd48 added

comment:20 Changed 9 years ago by Karl-Dieter Crisman

Okay, Burcin figured out what he forgot here :) and most of the rest should be easily fixable.

Last edited 9 years ago by Karl-Dieter Crisman (previous) (diff)

comment:21 Changed 9 years ago by Karl-Dieter Crisman

Okay, everything fixed except a segfault on beta which is exactly the same one which this fixed. Burcin says that this needs to be handled properly and so

static ex beta_evalf(const ex & x, const ex & y, PyObject* parent)
{
        if (is_exactly_a<numeric>(x) && is_exactly_a<numeric>(y)) {
                try {
                        return exp(lgamma(ex_to<numeric>(x))+lgamma(ex_to<numeric>(y))-lgamma(ex_to<numeric>(x+y)));
                } catch (const dunno &e) { }
        }

        return beta(x,y).hold();
}

in ginac/inifcns_gamma.cpp needs to be worked on in addition to other stuff.

Changed 9 years ago by Karl-Dieter Crisman

Attachment: trac_12289-more-alg.patch added

comment:22 Changed 9 years ago by Karl-Dieter Crisman

Description: modified (diff)

Patchbot, apply trac_12289-evalf_dictionary_rebase.patch , trac_12289-add_algorithm-rebase.patch, trac_12289-py_float-fix-rebase.patch, and trac_12289-more-alg.patch

Changed 9 years ago by Burcin Erocal

Changed 9 years ago by Burcin Erocal

comment:23 Changed 9 years ago by Burcin Erocal

Dependencies: #13933, #4102
Description: modified (diff)
Status: needs_workneeds_review

New patches up. All doctests pass in sage/{symbolics,calculus}/.

comment:24 in reply to:  9 Changed 9 years ago by Karl-Dieter Crisman

So that we don't forget to fix it later:

sage: erf(1).n(algorithm='foo')
0.842700792949715
sage: erf(0)
0
sage: parent(erf(0))
Integer Ring
sage: erf(0).n(algorithm='foo')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/Users/mcneil/sagedev/sage-5.1.beta0b/sage-5.1.beta0/devel/sage-hack2/sage/<ipython console> in <module>()

/Users/mcneil/sagedev/sage-5.1.beta0b/sage-5.1.beta0/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.numerical_approx (sage/structure/element.c:4988)()

TypeError: numerical_approx() got an unexpected keyword argument 'algorithm'

This is now #14778. Shouldn't impact this being merged.

By which to say, this passes all tests. The most recent fixes are fine. Burcin, what would need to be reviewed still?

comment:25 Changed 9 years ago by Karl-Dieter Crisman

This (or possibly a closely related patch) fails a doctest, actually. Sorry if this isn't the precise ticket.

sage -t sage/misc/sagedoc.py
**********************************************************************
File "sage/misc/sagedoc.py", line 22, in sage.misc.sagedoc
Failed example:
    for line in open(docfilename):
          if "#sage.symbolic.expression.Expression.N" in line:
              print line
Expected:
    <tt class="descname">N</tt><big>(</big><em>prec=None</em>, <em>digits=None</em><big>)</big>...
Got:
    <tt class="descname">N</tt><big>(</big><em>prec=None</em>, <em>digits=None</em>, <em>algorithm=None</em><big>)</big><a class="headerlink" href="#sage.symbolic.expression.Expression.N" title="Permalink to this definition">¶</a></dt>
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of   3 in sage.misc.sagedoc
    [73 tests, 1 failure, 9.81 s]

Patchbot, apply trac_12289-evalf_dictionary_rebase.take2.patch , trac_12289-add_algorithm-rebase.patch , trac_12289-more-alg.patch trac_12289-last_touches.patch

comment:26 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:27 Changed 9 years ago by Eviatar Bach

Dependencies: #13933, #4102#13933, #4102, #15198

comment:28 Changed 9 years ago by Eviatar Bach

Summary: pass algorithm argument to custom numeric evalution methodspass algorithm argument to custom numeric evaluation methods

comment:29 in reply to:  25 Changed 9 years ago by Karl-Dieter Crisman

Replying to kcrisman:

This (or possibly a closely related patch) fails a doctest, actually. Sorry if this isn't the precise ticket.

sage -t sage/misc/sagedoc.py
**********************************************************************
File "sage/misc/sagedoc.py", line 22, in sage.misc.sagedoc
Failed example:
    for line in open(docfilename):
          if "#sage.symbolic.expression.Expression.N" in line:
              print line
Expected:
    <tt class="descname">N</tt><big>(</big><em>prec=None</em>, <em>digits=None</em><big>)</big>...
Got:
    <tt class="descname">N</tt><big>(</big><em>prec=None</em>, <em>digits=None</em>, <em>algorithm=None</em><big>)</big><a class="headerlink" href="#sage.symbolic.expression.Expression.N" title="Permalink to this definition">¶</a></dt>
    <BLANKLINE>

Yeah, I'm pretty sure this would do it, because of

def _numerical_approx(self, prec=None, digits=None, algorithm=None): 

in the symbolic expression file.

comment:30 Changed 9 years ago by Frédéric Chapoton

for the patchbots:

apply trac_12289-evalf_dictionary_rebase.take2.patch trac_12289-add_algorithm-rebase.patch trac_12289-more-alg.patch trac_12289-last_touches.patch

comment:31 Changed 9 years ago by Burcin Erocal

This ticket depends on an SPKG update. The patchbot cannot handle those ATM.

IIRC, the pynac update is complete and waiting for review. See #15198 for details.

comment:32 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:33 Changed 9 years ago by Jean-Pierre Flori

Branch: u/jpflori/ticket/12289
Commit: 5d1fd3390700a4ad7521d14bad4d7b08f30e7edb
Description: modified (diff)

New commits:

5aa39ceUse dictionary for custom numerical evaluation.
d86cef5Add algorithm keyword to custom numerical evaluation.
e08b282Add missing algorithm keywords for custom numerical evaluation.
008a54fFix random issues caused by _evalf_ convention change.
5d1fd33Fix rebasing error.

comment:34 Changed 9 years ago by git

Commit: 5d1fd3390700a4ad7521d14bad4d7b08f30e7edbb658858f25aef3c0fcc32fc72423aaa387305979

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

b658858Proper fix for the rebasing issue.

comment:35 Changed 9 years ago by git

Commit: b658858f25aef3c0fcc32fc72423aaa387305979da159a053fb8b6de61f9e17c8c74991d9a44a59d

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

da159a0Add missing fix for algorithm keyword for custom numerical evaluation.

comment:36 Changed 9 years ago by Jean-Pierre Flori

Reviewers: Karl-Dieter Crisman, Doug S. MacNeil, Benjamin JonesKarl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre Flori
Work issues: doctests, documentationblankline

I just rebased the old patches and fixed an additional missing new keyword. I'm happy with the changes. There is still the blankline issue of comment:29.

comment:37 Changed 9 years ago by git

Commit: da159a053fb8b6de61f9e17c8c74991d9a44a59def6d2de56ff56851710ecfdc9dcdc4adfec91c08

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

ef6d2deFix documentation for numerical_approximation.

comment:38 Changed 9 years ago by Jean-Pierre Flori

In fact the doctest was expected to change... so needs review for the two basic changes I made. What was done before was already reviewed and I'm happy in case it was not.


New commits:

ef6d2deFix documentation for numerical_approximation.

comment:39 Changed 9 years ago by Jean-Pierre Flori

Cc: Volker Braun added

comment:40 Changed 9 years ago by Karl-Dieter Crisman

I can't do actual testing now (or any time soon, I've really fallen off the development radar screen) but these minor changes in the last two commits are completely fine as long as the doc change fixes the problem. Hopefully we'll actually start implementing some of these algorithms now...

comment:41 Changed 9 years ago by Jean-Pierre Flori

Status: needs_reviewpositive_review

Testing will be done automatically by the patchbots, so I'll take your comment as positive review.

comment:42 Changed 9 years ago by Volker Braun

Branch: u/jpflori/ticket/12289ef6d2de56ff56851710ecfdc9dcdc4adfec91c08
Resolution: fixed
Status: positive_reviewclosed

comment:43 Changed 8 years ago by Karl-Dieter Crisman

Commit: ef6d2de56ff56851710ecfdc9dcdc4adfec91c08
Reviewers: Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre FloriKarl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori
Work issues: blankline

comment:44 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

Is there any particular reason that this was done for _evalf_ and not plain _eval_?

Version 0, edited 8 years ago by Jeroen Demeyer (next)

comment:45 Changed 8 years ago by Jeroen Demeyer

Similarly: shouldn't __call__ support an algorithm argument such that one could do

sage: f(0.12345, algorithm="foobar")
Note: See TracTickets for help on using tickets.