Opened 5 years ago

Closed 21 months ago

#17531 closed enhancement (wontfix)

allow algorithm keyword when calling BuiltinFunction

Reported by: rws Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords: symbolic functions
Cc: jdemeyer Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/17531-1 (Commits) Commit: b7e88d4ec6f74aa43f9fb5c8d9c54218492cba35
Dependencies: Stopgaps:

Description (last modified by rws)

As per http://trac.sagemath.org/ticket/17489#comment:17 and http://trac.sagemath.org/ticket/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.

Change History (42)

comment:1 Changed 5 years ago by rws

  • Cc jdemeyer added
  • Description modified (diff)
  • Summary changed from allow algorithm keyword when _call_()ing BuiltinFunction to allow algorithm keyword when calling BuiltinFunction

comment:2 Changed 5 years ago by rws

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

comment:3 Changed 5 years ago by jdemeyer

f = MyFunction()

instead of

f = MyFunction

Also, __evalf__ should be _evalf_.

comment:4 Changed 5 years ago by rws

  • Description modified (diff)

comment:5 follow-up: Changed 5 years ago by rws

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)

comment:6 Changed 5 years ago by rws

  • Branch set to u/rws/allow_algorithm_keyword_when_calling_builtinfunction

comment:7 Changed 5 years ago by rws

  • Branch u/rws/allow_algorithm_keyword_when_calling_builtinfunction deleted
  • Description modified (diff)

comment:8 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Branch set to u/rws/allow_algorithm_keyword_when_calling_builtinfunction
  • Commit set to eaed76a81b98c111bd77c05ccce745793e01b85e

New commits:

eaed76a17531: add _algorithm field to cdef class Function

comment:9 Changed 5 years ago by rws

  • Status changed from new to needs_review

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)

comment:10 in reply to: ↑ 5 Changed 5 years ago by rws

Replying to rws:

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

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

comment:11 follow-up: Changed 5 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 years ago by rws

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.

comment:13 follow-up: Changed 5 years ago by kcrisman

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by rws

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.

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

comment:15 in reply to: ↑ 14 Changed 5 years ago by rws

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.

comment:16 Changed 5 years ago by kcrisman

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

comment:17 Changed 5 years ago by kcrisman

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.

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

comment:18 Changed 5 years ago by jdemeyer

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

comment:19 Changed 5 years ago by rws

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

comment:20 Changed 5 years ago by git

  • Commit changed from eaed76a81b98c111bd77c05ccce745793e01b85e to d8f09d0fec7e08d234b6154b5065cab1d3c1ad89

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

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

comment:21 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I really don't like this:

self._algorithm = algorithm

comment:22 follow-up: Changed 5 years ago by jdemeyer

I would put this

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

right before calling Ginac (towards the end of Function.__call__).

comment:23 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by rws

Replying to jdemeyer:

I would put this

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

right before calling Ginac (towards the end of Function.__call__).

But when you do sin(x,algorithm='foo') the Function.__call__ doesn't even get called (EDIT: because BuiltinFunction.__call__ doesn't call super) so no error is thrown.

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

comment:24 Changed 5 years ago by git

  • Commit changed from d8f09d0fec7e08d234b6154b5065cab1d3c1ad89 to 81fab64d599d6e3e0dc1d142b86998b04042562b

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

81fab6417531: just pass algorithm via call to _evalf_

comment:25 in reply to: ↑ 21 Changed 5 years ago by rws

Replying to jdemeyer:

I really don't like this:

self._algorithm = algorithm

Indeed, I don't need a class field for this, thanks for the hint. I don't see a better solution for the other issue mentioned, however.

comment:26 Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

comment:27 in reply to: ↑ 23 ; follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to rws:

But when you do sin(x,algorithm='foo') the Function.__call__ doesn't even get called (EDIT: because BuiltinFunction.__call__ doesn't call super) so no error is thrown.

In any case, the place where the exception is raised now isn't correct.

comment:28 in reply to: ↑ 27 Changed 5 years ago by rws

Replying to jdemeyer:

In any case, the place where the exception is raised now isn't correct.

Please be more verbose. It concerns GinacFunctions so I think it's placed right, and it obviously works. So I'm probably missing something that you know, and I don't want to fiddle with your BuiltinFunction code.

comment:29 Changed 5 years ago by rws

  • Status changed from needs_work to needs_info

comment:30 Changed 5 years ago by rws

#16812 depends on this.

comment:31 Changed 5 years ago by git

  • Commit changed from 81fab64d599d6e3e0dc1d142b86998b04042562b to b838310bd8b22284f60fdeff03a1de7b5cd9e49c

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

3080e76Merge branch 'develop' into t/17531/allow_algorithm_keyword_when_calling_builtinfunction
b83831017531: move exception raising out of GinacFunction.__call__

comment:32 Changed 5 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.8
  • Status changed from needs_info to needs_review

Let's recap. What I missed is that while sin(x) and sin(1.1) do not call Function.__call__ sin(0) does. Regarding sin(1.1) BuiltinFunction::__call__ calls sage.rings.real_mpfr.RealLiteral.sin directly so any check for 'ginac' would be nonsense. Should we check for any other algorithm keyword there?

comment:33 Changed 5 years ago by rws

  • Description modified (diff)

comment:34 Changed 4 years ago by rws

  • Milestone changed from sage-6.8 to sage-6.10

Ping?

comment:35 Changed 4 years ago by rws

  • Branch changed from u/rws/allow_algorithm_keyword_when_calling_builtinfunction to u/rws/17531-1

comment:36 Changed 3 years ago by rws

  • Commit changed from b838310bd8b22284f60fdeff03a1de7b5cd9e49c to acedb884403e9363471d71132b68c38a3c874606
  • Status changed from needs_review to needs_work

Seems the recent gamma_inc changes trigger test failures.


New commits:

acedb8817531: allow algorithm keyword when calling BuiltinFunction

comment:37 Changed 3 years ago by git

  • Commit changed from acedb884403e9363471d71132b68c38a3c874606 to ebac431f4024ec7b92a44138f9dbeb8c6cd2d6bf

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

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

comment:38 Changed 3 years ago by rws

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

comment:39 Changed 3 years ago by git

  • Commit changed from ebac431f4024ec7b92a44138f9dbeb8c6cd2d6bf to b7e88d4ec6f74aa43f9fb5c8d9c54218492cba35

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

b7e88d4add doctest tolerances

comment:40 Changed 3 years ago by rws

  • Status changed from needs_work to needs_review

comment:41 Changed 2 years ago by rws

  • Milestone changed from sage-6.10 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

I'll close this as wontfix, my argument is in https://trac.sagemath.org/ticket/17489#comment:48

comment:42 Changed 21 months ago by vdelecroix

  • Resolution set to wontfix
  • Status changed from positive_review to closed

closing positively reviewed duplicates

Note: See TracTickets for help on using tickets.