Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#12289 closed enhancement (fixed)

pass algorithm argument to custom numeric evaluation methods

Reported by: burcin Owned by: burcin
Priority: major Milestone: sage-6.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: Karl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: ef6d2de (Commits) Commit:
Dependencies: #13933, #4102, #15198 Stopgaps:

Description (last modified by jdemeyer)

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 8 years ago.
trac_12289-add_algorithm_arg.patch (9.0 KB) - added by burcin 8 years ago.
trac_12289-example.patch (456 bytes) - added by burcin 8 years ago.
trac_12289-evalf_dictionary_rebase.patch (4.2 KB) - added by benjaminfjones 7 years ago.
rebase to sage-5.0 + pynac-0.2.4 spkg
trac_12289_py_float_fix.patch (683 bytes) - added by benjaminfjones 7 years ago.
fix py_float delcaration
trac_12289-py_float-fix-rebase.patch (663 bytes) - added by kcrisman 6 years ago.
trac_12289-add_algorithm-rebase.patch (8.3 KB) - added by kcrisman 6 years ago.
trac_12289-more-alg.patch (5.1 KB) - added by kcrisman 6 years ago.
trac_12289-evalf_dictionary_rebase.take2.patch (4.2 KB) - added by burcin 6 years ago.
trac_12289-last_touches.patch (3.0 KB) - added by burcin 6 years ago.

Download all attachments as: .zip

Change History (55)

Changed 8 years ago by burcin

Changed 8 years ago by burcin

Changed 8 years ago by burcin

comment:1 Changed 8 years ago by burcin

  • Cc benjaminfjones 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 8 years ago by benjaminfjones

  • 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 benjaminfjones

  • Cc dsm added
  • Keywords sd40.5 added

comment:4 Changed 7 years ago by benjaminfjones

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 7 years ago by kcrisman

  • Cc kcrisman added

comment:6 Changed 7 years ago by kini

  • Cc kini added

Changed 7 years ago by benjaminfjones

rebase to sage-5.0 + pynac-0.2.4 spkg

Changed 7 years ago by benjaminfjones

fix py_float delcaration

comment:7 follow-up: Changed 7 years ago by benjaminfjones

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 7 years ago by kcrisman

  • 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 follow-up: Changed 7 years ago by dsm

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 6 years ago by kcrisman

  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 6 years ago by eviatarbach

  • Cc eviatarbach added

comment:12 Changed 6 years ago by kcrisman

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

comment:13 Changed 6 years ago by kcrisman

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 kcrisman

Changed 6 years ago by kcrisman

comment:14 Changed 6 years ago by kcrisman

  • Description modified (diff)

comment:15 Changed 6 years ago by kcrisman

  • 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 6 years ago by kcrisman

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 kcrisman

  • Authors changed from Burcin Erocal to Burcin Erocal, Benjamin Jones
  • Reviewers set to Karl-Dieter 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 kcrisman

  • Status changed from needs_review to needs_work

comment:19 Changed 6 years ago by eviatarbach

  • Keywords sd48 added

comment:20 Changed 6 years ago by kcrisman

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

Last edited 6 years ago by kcrisman (previous) (diff)

comment:21 Changed 6 years ago by kcrisman

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 kcrisman

comment:22 Changed 6 years ago by kcrisman

  • 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 6 years ago by burcin

comment:23 Changed 6 years ago by burcin

  • 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 kcrisman

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 follow-up: Changed 6 years ago by 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>
**********************************************************************
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 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:27 Changed 6 years ago by eviatarbach

  • Dependencies changed from #13933, #4102 to #13933, #4102, #15198

comment:28 Changed 6 years ago by eviatarbach

  • 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 kcrisman

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 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 6 years ago by burcin

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 vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:33 Changed 6 years ago by jpflori

  • Branch set to u/jpflori/ticket/12289
  • Commit set to 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 6 years ago by git

  • Commit changed from 5d1fd3390700a4ad7521d14bad4d7b08f30e7edb to b658858f25aef3c0fcc32fc72423aaa387305979

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

b658858Proper fix for the rebasing issue.

comment:35 Changed 6 years ago by git

  • Commit changed from b658858f25aef3c0fcc32fc72423aaa387305979 to da159a053fb8b6de61f9e17c8c74991d9a44a59d

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

da159a0Add missing fix for algorithm keyword for custom numerical evaluation.

comment:36 Changed 6 years ago by jpflori

  • Reviewers changed from Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones to Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre 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 git

  • Commit changed from da159a053fb8b6de61f9e17c8c74991d9a44a59d to ef6d2de56ff56851710ecfdc9dcdc4adfec91c08

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

ef6d2deFix documentation for numerical_approximation.

comment:38 Changed 6 years ago by jpflori

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 6 years ago by jpflori

  • Cc vbraun added

comment:40 Changed 6 years ago by kcrisman

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 jpflori

  • 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 vbraun

  • 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 kcrisman

  • Commit ef6d2de56ff56851710ecfdc9dcdc4adfec91c08 deleted
  • Reviewers changed from Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre Flori to Karl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori
  • Work issues blankline deleted

comment:44 Changed 5 years ago by jdemeyer

  • Description modified (diff)

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

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:45 Changed 5 years ago by jdemeyer

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.