#12289 closed enhancement (fixed)
pass algorithm argument to custom numeric evaluation methods
Reported by:  Burcin Erocal  Owned by:  Burcin Erocal 

Priority:  major  Milestone:  sage6.2 
Component:  symbolics  Keywords:  pynac sd35.5 sd40.5 sd48 
Cc:  Benjamin Jones, D.S. McNeil, KarlDieter Crisman, Keshav Kini, Eviatar Bach, Volker Braun  Merged in:  
Authors:  Burcin Erocal, Benjamin Jones  Reviewers:  KarlDieter Crisman, Douglas MacNeil, Benjamin Jones, JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  ef6d2de (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #13933, #4102, #15198  Stopgaps: 
Description (last modified by )
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)
Change History (55)
Changed 11 years ago by
Attachment:  trac_12289evalf_dictionary.patch added 

Changed 11 years ago by
Attachment:  trac_12289add_algorithm_arg.patch added 

Changed 11 years ago by
Attachment:  trac_12289example.patch added 

comment:1 Changed 11 years ago by
Cc:  Benjamin Jones added 

comment:2 Changed 11 years ago by
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
Cc:  D.S. McNeil added 

Keywords:  sd40.5 added 
comment:4 Changed 11 years ago by
I'm trying to rebase the patch trac_12289evalf_dictionary.patch to sage5.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 sage5.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
Cc:  KarlDieter Crisman added 

comment:6 Changed 11 years ago by
Cc:  Keshav Kini added 

Changed 11 years ago by
Attachment:  trac_12289evalf_dictionary_rebase.patch added 

rebase to sage5.0 + pynac0.2.4 spkg
comment:7 followup: 10 Changed 11 years ago by
I've been working on this ticket today at sd40.5. I've got burcin's changes working in sage5.0 with a caveat. Here's how to get the example in comment:1 working in sage5.0:
 install the patched pynac0.2.4.p1 spkg at http://sage.math.washington.edu/home/bjones/pynac0.2.4.p1.spkg. this spkg is the pynac0.2.4 spkg from #12950 which has been patched to include the changes from burcin's
pynacwip
branch of pynac mentioned above.
 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
 apply the patches to
$SAGE_ROOT/devel/sage
sage b
comment:8 Changed 11 years ago by
Description:  modified (diff) 

Patchbot, though, should only apply trac_12289evalf_dictionary_rebase.patch, trac_12289add_algorithm_arg.patch, and trac_12289_py_float_fix.patch
comment:9 followup: 24 Changed 11 years ago by
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/sage5.1.beta0b/sage5.1.beta0/devel/sagehack2/sage/<ipython console> in <module>() /Users/mcneil/sagedev/sage5.1.beta0b/sage5.1.beta0/local/lib/python2.7/sitepackages/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 Changed 10 years ago by
 install the patched pynac0.2.4.p1 spkg at http://sage.math.washington.edu/home/bjones/pynac0.2.4.p1.spkg. this spkg is the pynac0.2.4 spkg from #12950 which has been patched to include the changes from burcin's
pynacwip
branch of pynac mentioned above.
Is this stuff in the current Pynac 0.2.6?
 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.
 apply the patches to
$SAGE_ROOT/devel/sage
sage b
comment:11 Changed 10 years ago by
Cc:  Eviatar Bach added 

comment:13 Changed 9 years ago by
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
Attachment:  trac_12289py_floatfixrebase.patch added 

Changed 9 years ago by
Attachment:  trac_12289add_algorithmrebase.patch added 

comment:14 Changed 9 years ago by
Description:  modified (diff) 

comment:15 Changed 9 years ago by
Description:  modified (diff) 

Patchbot, apply trac_12289evalf_dictionary_rebase.patch , trac_12289add_algorithmrebase.patch , and trac_12289py_floatfixrebase.patch
comment:16 Changed 9 years ago by
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
Authors:  Burcin Erocal → Burcin Erocal, Benjamin Jones 

Reviewers:  → KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones 
Status:  new → needs_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
Status:  needs_review → needs_work 

comment:19 Changed 9 years ago by
Keywords:  sd48 added 

comment:20 Changed 9 years ago by
Okay, Burcin figured out what he forgot here :) and most of the rest should be fixable.
comment:21 Changed 9 years ago by
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
Attachment:  trac_12289morealg.patch added 

comment:22 Changed 9 years ago by
Description:  modified (diff) 

Patchbot, apply trac_12289evalf_dictionary_rebase.patch , trac_12289add_algorithmrebase.patch, trac_12289py_floatfixrebase.patch, and trac_12289morealg.patch
Changed 9 years ago by
Attachment:  trac_12289evalf_dictionary_rebase.take2.patch added 

Changed 9 years ago by
Attachment:  trac_12289last_touches.patch added 

comment:23 Changed 9 years ago by
Dependencies:  → #13933, #4102 

Description:  modified (diff) 
Status:  needs_work → needs_review 
New patches up. All doctests pass in sage/{symbolics,calculus}/
.
comment:24 Changed 9 years ago by
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/sage5.1.beta0b/sage5.1.beta0/devel/sagehack2/sage/<ipython console> in <module>() /Users/mcneil/sagedev/sage5.1.beta0b/sage5.1.beta0/local/lib/python2.7/sitepackages/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 followup: 29 Changed 9 years ago by
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_12289evalf_dictionary_rebase.take2.patch , trac_12289add_algorithmrebase.patch , trac_12289morealg.patch trac_12289last_touches.patch
comment:26 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:27 Changed 9 years ago by
Dependencies:  #13933, #4102 → #13933, #4102, #15198 

comment:28 Changed 9 years ago by
Summary:  pass algorithm argument to custom numeric evalution methods → pass algorithm argument to custom numeric evaluation methods 

comment:29 Changed 9 years ago by
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
for the patchbots:
apply trac_12289evalf_dictionary_rebase.take2.patch trac_12289add_algorithmrebase.patch trac_12289morealg.patch trac_12289last_touches.patch
comment:31 Changed 9 years ago by
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
Milestone:  sage6.1 → sage6.2 

comment:33 Changed 9 years ago by
Branch:  → u/jpflori/ticket/12289 

Commit:  → 5d1fd3390700a4ad7521d14bad4d7b08f30e7edb 
Description:  modified (diff) 
comment:34 Changed 9 years ago by
Commit:  5d1fd3390700a4ad7521d14bad4d7b08f30e7edb → b658858f25aef3c0fcc32fc72423aaa387305979 

Branch pushed to git repo; I updated commit sha1. New commits:
b658858  Proper fix for the rebasing issue.

comment:35 Changed 9 years ago by
Commit:  b658858f25aef3c0fcc32fc72423aaa387305979 → da159a053fb8b6de61f9e17c8c74991d9a44a59d 

Branch pushed to git repo; I updated commit sha1. New commits:
da159a0  Add missing fix for algorithm keyword for custom numerical evaluation.

comment:36 Changed 9 years ago by
Reviewers:  KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones → KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones, JeanPierre Flori 

Work issues:  doctests, documentation → blankline 
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
Commit:  da159a053fb8b6de61f9e17c8c74991d9a44a59d → ef6d2de56ff56851710ecfdc9dcdc4adfec91c08 

Branch pushed to git repo; I updated commit sha1. New commits:
ef6d2de  Fix documentation for numerical_approximation.

comment:38 Changed 9 years ago by
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:
ef6d2de  Fix documentation for numerical_approximation.

comment:39 Changed 9 years ago by
Cc:  Volker Braun added 

comment:40 Changed 9 years ago by
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
Status:  needs_review → positive_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
Branch:  u/jpflori/ticket/12289 → ef6d2de56ff56851710ecfdc9dcdc4adfec91c08 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:43 Changed 8 years ago by
Commit:  ef6d2de56ff56851710ecfdc9dcdc4adfec91c08 

Reviewers:  KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones, JeanPierre Flori → KarlDieter Crisman, Douglas MacNeil, Benjamin Jones, JeanPierre Flori 
Work issues:  blankline 
comment:44 Changed 8 years ago by
Description:  modified (diff) 

Is there any particular reason that this was done for _evalf_
and not also plain _eval_
?
comment:45 Changed 8 years ago by
Similarly: shouldn't __call__
support an algorithm
argument such that one could do
sage: f(0.12345, algorithm="foobar")
The
evalf_dict
branch of Pynac available here, with the attached patches applied to the Sage library, allows this:The docstrings for
_convert()
etc. methods of symbolic expressions need to be updated to reflect these changes.