Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pass algorithm argument to custom numeric evaluation methods #12289

Closed
burcin opened this issue Jan 10, 2012 · 65 comments
Closed

pass algorithm argument to custom numeric evaluation methods #12289

burcin opened this issue Jan 10, 2012 · 65 comments

Comments

@burcin
Copy link

burcin commented Jan 10, 2012

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

Depends on #13933
Depends on #4102
Depends on #15198

CC: @benjaminfjones @sagetrac-dsm @kcrisman @kini @eviatarbach @vbraun

Component: symbolics

Keywords: pynac sd35.5 sd40.5 sd48

Author: Burcin Erocal, Benjamin Jones

Branch: ef6d2de

Reviewer: Karl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori

Issue created by migration from https://trac.sagemath.org/ticket/12289

@burcin burcin added this to the sage-5.11 milestone Jan 10, 2012
@burcin burcin self-assigned this Jan 10, 2012
@burcin
Copy link
Author

burcin commented Jan 10, 2012

Attachment: trac_12289-evalf_dictionary.patch.gz

@burcin
Copy link
Author

burcin commented Jan 10, 2012

@burcin
Copy link
Author

burcin commented Jan 10, 2012

comment:1

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.

@benjaminfjones
Copy link
Contributor

comment:2

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

@benjaminfjones
Copy link
Contributor

Changed keywords from pynac to pynac sd35.5

@benjaminfjones
Copy link
Contributor

Changed keywords from pynac sd35.5 to pynac sd35.5 sd40.5

@benjaminfjones
Copy link
Contributor

comment:4

I'm trying to rebase the patch attachment: 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?)

@benjaminfjones
Copy link
Contributor

Attachment: trac_12289-evalf_dictionary_rebase.patch.gz

rebase to sage-5.0 + pynac-0.2.4 spkg

@benjaminfjones
Copy link
Contributor

Attachment: trac_12289_py_float_fix.patch.gz

fix py_float delcaration

@benjaminfjones
Copy link
Contributor

comment:7

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 update to Pynac 0.2.4 #12950 which has been patched to include the changes from burcin's pynac-wip branch of pynac mentioned above.

  2. apply all patches at update to Pynac 0.2.4 #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

  3. apply the patches to $SAGE_ROOT/devel/sage

  4. sage -b

@kcrisman
Copy link
Member

comment:8

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

@kcrisman

This comment has been minimized.

@sagetrac-dsm
Copy link
Mannequin

sagetrac-dsm mannequin commented May 28, 2012

comment:9

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'

@kcrisman
Copy link
Member

comment:10
  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 update to Pynac 0.2.4 #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 update to Pynac 0.2.4 #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

  2. sage -b

@kcrisman
Copy link
Member

comment:12

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

@kcrisman
Copy link
Member

comment:13

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.

@kcrisman
Copy link
Member

Attachment: trac_12289-py_float-fix-rebase.patch.gz

@kcrisman
Copy link
Member

comment:14

Attachment: trac_12289-add_algorithm-rebase.patch.gz

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member

comment:15

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

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member

comment:16

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.

@kcrisman
Copy link
Member

Work Issues: doctests, documentation

@kcrisman
Copy link
Member

comment:17

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!

@kcrisman
Copy link
Member

Reviewer: Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones

@jpflori
Copy link

jpflori commented Feb 20, 2014

Commit: 5d1fd33

@jpflori
Copy link

jpflori commented Feb 20, 2014

Branch: u/jpflori/ticket/12289

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

Changed commit from 5d1fd33 to b658858

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

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

b658858Proper fix for the rebasing issue.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

Changed commit from b658858 to da159a0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

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

da159a0Add missing fix for algorithm keyword for custom numerical evaluation.

@jpflori
Copy link

jpflori commented Feb 20, 2014

Changed work issues from doctests, documentation to blankline

@jpflori
Copy link

jpflori commented Feb 20, 2014

comment:36

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.

@jpflori
Copy link

jpflori commented Feb 20, 2014

Changed reviewer from Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones to Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre Flori

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

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

ef6d2deFix documentation for numerical_approximation.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2014

Changed commit from da159a0 to ef6d2de

@jpflori
Copy link

jpflori commented Feb 20, 2014

comment:38

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.

@kcrisman
Copy link
Member

comment:40

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

@jpflori
Copy link

jpflori commented Feb 28, 2014

comment:41

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

@vbraun
Copy link
Member

vbraun commented Mar 4, 2014

Changed branch from u/jpflori/ticket/12289 to ef6d2de

@kcrisman
Copy link
Member

Changed commit from ef6d2de to none

@kcrisman
Copy link
Member

Changed reviewer from Karl-Dieter Crisman, Doug S. MacNeil, Benjamin Jones, Jean-Pierre Flori to Karl-Dieter Crisman, Douglas MacNeil, Benjamin Jones, Jean-Pierre Flori

@kcrisman
Copy link
Member

Changed work issues from blankline to none

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:44

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

@jdemeyer
Copy link

comment:45

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

sage: f(0.12345, algorithm="foobar")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants