allow algorithm keyword when calling BuiltinFunction

[rwst]

As per #17489 comment:17 and #17489 comment:23, the ticket should improve the BuiltinFunction class, such that subclassed function classes that have an algorithm=... keyword given via function call will automatically have this keyword inserted into any __evalf__ member function call.

This means the following should be possible without changes to subclassedfunction.__call__:

sage: from sage.symbolic.function import BuiltinFunction
sage: class MyFunction(BuiltinFunction):
....:    def __init__(self):
....:        BuiltinFunction.__init__(self, 'test', nargs=1)
....:    def _evalf_(self, x, **kwds):
....:        algorithm = kwds.get('algorithm', None)
....:        if algorithm == 'algoA':
....:            return 1.0
....:        else:
....:            return 0.0
....:    def _eval_(self, x):
....:         return self._evalf_try_(x)
sage: f = MyFunction()
sage: f(0.0, algorithm='algoA')
1.00000000000000
sage: f(0.0)
0.000000000000000

At the moment, as with all(?) symbolic functions we get

sage: f(x, algorithm='algoB')
TypeError: __call__() got an unexpected keyword argument 'algorithm'
sage: sin(x, algorithm='algoB')
TypeError: __call__() got an unexpected keyword argument 'algorithm'

So the ticket should also give a better error message for existing functions.

#16812 and #17489 depend on this.

CC: @jdemeyer

Component: symbolics

Keywords: symbolic functions

Author: Ralf Stephan

Branch/Commit: u/rws/17531-1 @ b7e88d4

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

[rwst]

comment:2

Is there something missing in the example?

sage: f(x)
Traceback (most recent call last):
  File "<ipython-input-10-963bfc488912>", line 1, in <module>
    f(x)
  File "sage/symbolic/function.pyx", line 942, in sage.symbolic.function.BuiltinFunction.__init__ (build/cythonized/sage/symbolic/function.cpp:9902)
    Function.__init__(self, name, nargs, latex_name, conversions,
  File "sage/symbolic/function.pyx", line 110, in sage.symbolic.function.Function.__init__ (build/cythonized/sage/symbolic/function.cpp:3655)
    if not self._is_registered():
  File "sage/symbolic/function.pyx", line 1042, in sage.symbolic.function.BuiltinFunction._is_registered (build/cythonized/sage/symbolic/function.cpp:10966)
    try:
TypeError: expected string or Unicode object, sage.symbolic.expression.Expression found

[jdemeyer]

comment:3

f = MyFunction()

instead of

f = MyFunction

Also, __evalf__ should be _evalf_.

[rwst]

comment:5

Isn't this a bug? Hold as no effect:

sage: from sage.symbolic.function import BuiltinFunction
sage: class MyFunction(BuiltinFunction):
....:    def __init__(self, name):
....:        BuiltinFunction.__init__(self, name, nargs=1)
....:    def _evalf_(self, x, parent=None, algorithm=None):
....:        print algorithm
....:        return x
sage: f = MyFunction('f')
sage: ex = f(0.0, algorithm='algoA', hold=True); ex
algoA
0.000000000000000

sage: sin(0.0, hold=True)
sin(0.000000000000000)

[rwst]

Branch: u/rws/allow_algorithm_keyword_when_calling_builtinfunction

[rwst]

Changed branch from u/rws/allow_algorithm_keyword_when_calling_builtinfunction to none

[rwst]

Branch: u/rws/allow_algorithm_keyword_when_calling_builtinfunction

[rwst]

New commits:

eaed76a

17531: add _algorithm field to cdef class Function

[rwst]

Author: Ralf Stephan

[rwst]

Commit: eaed76a

[rwst]

comment:9

This means that any function now accepts the algorithm keyword which is better than any error message IMHO.

sage: sin(x, algorithm='algoB')
sin(x)

[rwst]

comment:10

Replying to @rwst:

Isn't this a bug? Hold as no effect:

I'll include a fix for this in #17489.

[kcrisman]

comment:11

sage: sin(x, algorithm='algoB')
sin(x)

General Sage behavior is to raise some sort of not implemented error or something "unknown algorithm" - we do this in a number of contexts.

[rwst]

comment:12

Replying to @kcrisman:

sage: sin(x, algorithm='algoB')
sin(x)

General Sage behavior is to raise some sort of not implemented error or something "unknown algorithm" - we do this in a number of contexts.

Actually, the sin function has no behaviour specified for the algorithm keyword and so, the outcome is undefined, meaning it could be either way. Certainly no such convention exists in the documentation. Pragmatically, it would be overkill to implement an algorithm registry for this reason.

[kcrisman]

comment:13

Well, right, in this case it should just say "undefined keyword" or something!

[rwst]

comment:14

Replying to @kcrisman:

Well, right, in this case it should just say "undefined keyword" or something!

If you so insist, please help with the following design decision. We could

• A) add a dictionary to the base class (Function) containing names of algorithms. The user (who writes subclasses of Function) may register names through subclass initialization. Unregistered names given in a __call__ argument result in error.
• B) make available the algorithm keyword only in a specific subclass of Function. The user would inherit from this subclass. Other existing classes retain their previous behaviour. The user is responsible to give an error in his class for unknown algorithms.
• C) disallow the algorithm keyword completely from any __call__ method of Function or its subclasses. This means that the end user cannot say factorial(10^6, algorithm=...) but should be able to say f(10^6, hold=True).n(algorithm=...). The function creator is responsible to give an error in his class for unknown algorithms.

Granted, C does make some sense in that all numeric issues are moved into _evalf_, and it works around shortfalls of A or B. But the end user will hate it. With A, there is additional code (clutter) in Function and the subclass must register its algorithm names. With B, it's not a general Function-wide solution.

EDIT: C does not make sense with functions returning int or polynomials and having different algorithms, unless the meaning of f in evalf or 'numerical approximation' is stretched beyond repair.

See also #15200 where Jeroen argues for hardcoding a few backend choices.

#17489 depends on this.

[rwst]

comment:15

EDIT: C does not make sense with functions returning int or polynomials and having different algorithms, unless the meaning of f in evalf or 'numerical approximation' is stretched beyond repair.

Nevertheless, C seems best.

[kcrisman]

comment:16

If you so insist, please help with the following design decision.

Hmm. I do kind of like A), actually. We already have the dictionary of conversions to other systems (Maxima, Mma, etc.), right? This seems very analogous. But it sounds like you have some concrete examples of some shortcomings. I do agree that the end user might not like C).

[kcrisman]

comment:17

Though maybe if all our other functions have C), as a little browsing suggests, maybe it is okay. For instance, in #17489 I'm not sure Jeroen is suggesting we must do this, just that it should be consistent.

[jdemeyer]

comment:18

I would propose D:

D) accept any algorithm in __call__, pass algorithm to _evalf_ (and possibly other methods) and do error handling "downstream". For example, the end of Function.__call__ uses Ginac, so there you could add

if algorithm is not None and algorithm != "ginac":
    raise ValueError("unknown algorithm %r for %s"%(algorithm,self))

[rwst]

comment:19

Ah OK. This can go into GinacFunction.__call__. Maybe I did not consider this solution because factorial, which needs to inherit from GinacFunction (because of #17547), needs the algorithm keyword. But this can be easily and naturally resolved by providing IntegerGinacFunction in #17489 with its own __call__.

[sagetrac-git]

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

4bac623	Merge branch 'develop' into t/17531/allow_algorithm_keyword_when_calling_builtinfunction
d8f09d0	17531: add check for any other than 'ginac' algorithm keywords in GinacFunction

[rwst]

comment:34

Ping?

[rwst]

Changed branch from u/rws/allow_algorithm_keyword_when_calling_builtinfunction to u/rws/17531-1

[rwst]

Changed commit from b838310 to acedb88

[rwst]

comment:36

Seems the recent gamma_inc changes trigger test failures.

---

New commits:

acedb88

17531: allow algorithm keyword when calling BuiltinFunction

[sagetrac-git]

Changed commit from acedb88 to ebac431

[sagetrac-git]

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

ebac431

Merge branch 'develop' into t/17531/17531-1

[rwst]

comment:38

**********************************************************************
File "src/sage/functions/other.py", line 1020, in sage.functions.other.Function_gamma_inc.__init__
Failed example:
    gamma_inc(CDF(0,1), 3)
Expected:
    0.0032085749933691158 + 0.012406185811871568*I
Got:
    0.003208574993369116 + 0.012406185811871567*I
**********************************************************************
File "src/sage/functions/other.py", line 1117, in sage.functions.other.Function_gamma_inc._evalf_
Failed example:
    gamma_inc(float(-1), float(-1))
Expected:
    (-0.8231640121031085+3.141592653589793j)
Got:
    (-0.8231640121031085-3.141592653589793j)
**********************************************************************
File "src/sage/functions/other.py", line 1119, in sage.functions.other.Function_gamma_inc._evalf_
Failed example:
    gamma_inc(RR(-1), RR(-1))
Expected:
    -0.823164012103109 + 3.14159265358979*I
Got:
    -0.823164012103109 - 3.14159265358979*I
**********************************************************************
File "src/sage/functions/other.py", line 1126, in sage.functions.other.Function_gamma_inc._evalf_
Failed example:
    r = gamma_inc(float(0), float(1)); r
Expected:
    0.21938393439552029
Got:
    0.21938393439552026

[sagetrac-git]

Changed commit from ebac431 to b7e88d4

[sagetrac-git]

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

b7e88d4

add doctest tolerances

[rwst]

comment:41

I'll close this as wontfix, my argument is in #17489 comment:48

[videlec]

comment:42

closing positively reviewed duplicates