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

Priority:  major  Milestone:  sage6.2 
Component:  symbolics  Keywords:  pynac sd35.5 sd40.5 sd48 
Cc:  benjaminfjones, dsm, kcrisman, kini, eviatarbach, vbraun  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)  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 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
comment:1 Changed 8 years ago by
 Cc benjaminfjones added
comment:2 Changed 8 years ago by
 Keywords sd35.5 added
I'm doing some testing of these patches along with rebasing #1173. Thanks Burcin!
comment:3 Changed 7 years ago by
 Cc dsm added
 Keywords sd40.5 added
comment:4 Changed 7 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 7 years ago by
 Cc kcrisman added
comment:6 Changed 7 years ago by
 Cc kini added
comment:7 followup: ↓ 10 Changed 7 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 7 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 7 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 in reply to: ↑ 7 Changed 6 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 6 years ago by
 Cc eviatarbach added
comment:12 Changed 6 years ago by
Some of these patches have bitrotted slightly. Updates coming up.
comment:13 Changed 6 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 6 years ago by
Changed 6 years ago by
comment:14 Changed 6 years ago by
 Description modified (diff)
comment:15 Changed 6 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 6 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 6 years ago by
 Reviewers set to KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones
 Status changed from new to needs_review
 Work issues set to 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 6 years ago by
 Status changed from needs_review to needs_work
comment:19 Changed 6 years ago by
 Keywords sd48 added
comment:20 Changed 6 years ago by
Okay, Burcin figured out what he forgot here :) and most of the rest should be fixable.
comment:21 Changed 6 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 6 years ago by
comment:22 Changed 6 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 6 years ago by
Changed 6 years ago by
comment:23 Changed 6 years ago by
 Dependencies set to #13933, #4102
 Description modified (diff)
 Status changed from needs_work to needs_review
New patches up. All doctests pass in sage/{symbolics,calculus}/
.
comment:24 in reply to: ↑ 9 Changed 6 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 6 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 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:27 Changed 6 years ago by
 Dependencies changed from #13933, #4102 to #13933, #4102, #15198
comment:28 Changed 6 years ago by
 Summary changed from pass algorithm argument to custom numeric evalution methods to pass algorithm argument to custom numeric evaluation methods
comment:29 in reply to: ↑ 25 Changed 6 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 6 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 6 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 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:33 Changed 6 years ago by
 Branch set to u/jpflori/ticket/12289
 Commit set to 5d1fd3390700a4ad7521d14bad4d7b08f30e7edb
 Description modified (diff)
comment:34 Changed 6 years ago by
 Commit changed from 5d1fd3390700a4ad7521d14bad4d7b08f30e7edb to b658858f25aef3c0fcc32fc72423aaa387305979
Branch pushed to git repo; I updated commit sha1. New commits:
b658858  Proper fix for the rebasing issue.

comment:35 Changed 6 years ago by
 Commit changed from b658858f25aef3c0fcc32fc72423aaa387305979 to 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 6 years ago by
 Reviewers changed from KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones to KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones, JeanPierre Flori
 Work issues changed from doctests, documentation to 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 6 years ago by
 Commit changed from da159a053fb8b6de61f9e17c8c74991d9a44a59d to ef6d2de56ff56851710ecfdc9dcdc4adfec91c08
Branch pushed to git repo; I updated commit sha1. New commits:
ef6d2de  Fix documentation for numerical_approximation.

comment:38 Changed 6 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 6 years ago by
 Cc vbraun added
comment:40 Changed 6 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 6 years ago by
 Status changed from needs_review to positive_review
Testing will be done automatically by the patchbots, so I'll take your comment as positive review.
comment:42 Changed 6 years ago by
 Branch changed from u/jpflori/ticket/12289 to ef6d2de56ff56851710ecfdc9dcdc4adfec91c08
 Resolution set to fixed
 Status changed from positive_review to closed
comment:43 Changed 5 years ago by
 Commit ef6d2de56ff56851710ecfdc9dcdc4adfec91c08 deleted
 Reviewers changed from KarlDieter Crisman, Doug S. MacNeil, Benjamin Jones, JeanPierre Flori to KarlDieter Crisman, Douglas MacNeil, Benjamin Jones, JeanPierre Flori
 Work issues blankline deleted
comment:44 Changed 5 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 5 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.