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:  sageduplicate/invalid/wontfix 
Component:  symbolics  Keywords:  symbolic functions 
Cc:  jdemeyer  Merged in:  
Authors:  Ralf Stephan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/rws/175311 (Commits)  Commit:  b7e88d4ec6f74aa43f9fb5c8d9c54218492cba35 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (42)
comment:1 Changed 5 years ago by
 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
comment:3 Changed 5 years ago by
f = MyFunction()
instead of
f = MyFunction
Also, __evalf__
should be _evalf_
.
comment:4 Changed 5 years ago by
 Description modified (diff)
comment:5 followup: ↓ 10 Changed 5 years ago by
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
 Branch set to u/rws/allow_algorithm_keyword_when_calling_builtinfunction
comment:7 Changed 5 years ago by
 Branch u/rws/allow_algorithm_keyword_when_calling_builtinfunction deleted
 Description modified (diff)
comment:8 Changed 5 years ago by
 Branch set to u/rws/allow_algorithm_keyword_when_calling_builtinfunction
 Commit set to eaed76a81b98c111bd77c05ccce745793e01b85e
New commits:
eaed76a  17531: add _algorithm field to cdef class Function

comment:9 Changed 5 years ago by
 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
comment:11 followup: ↓ 12 Changed 5 years ago by
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
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 followup: ↓ 14 Changed 5 years ago by
Well, right, in this case it should just say "undefined keyword" or something!
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 5 years ago by
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 ofFunction
) 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 ofFunction
or its subclasses. This means that the end user cannot sayfactorial(10^6, algorithm=...)
but should be able to sayf(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.
comment:15 in reply to: ↑ 14 Changed 5 years ago by
EDIT: C does not make sense with functions returning int or polynomials and having different algorithms, unless the meaning of
f
inevalf
or 'numerical approximation' is stretched beyond repair.
Nevertheless, C seems best.
comment:16 Changed 5 years ago by
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
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.
comment:18 Changed 5 years ago by
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
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
 Commit changed from eaed76a81b98c111bd77c05ccce745793e01b85e to d8f09d0fec7e08d234b6154b5065cab1d3c1ad89
comment:21 followup: ↓ 25 Changed 5 years ago by
 Status changed from needs_review to needs_work
I really don't like this:
self._algorithm = algorithm
comment:22 followup: ↓ 23 Changed 5 years ago by
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 ; followup: ↓ 27 Changed 5 years ago by
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.
comment:24 Changed 5 years ago by
 Commit changed from d8f09d0fec7e08d234b6154b5065cab1d3c1ad89 to 81fab64d599d6e3e0dc1d142b86998b04042562b
Branch pushed to git repo; I updated commit sha1. New commits:
81fab64  17531: just pass algorithm via call to _evalf_

comment:25 in reply to: ↑ 21 Changed 5 years ago by
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
 Status changed from needs_work to needs_review
comment:27 in reply to: ↑ 23 ; followup: ↓ 28 Changed 5 years ago by
 Status changed from needs_review to needs_work
Replying to rws:
But when you do
sin(x,algorithm='foo')
theFunction.__call__
doesn't even get called (EDIT: becauseBuiltinFunction.__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
Replying to jdemeyer:
In any case, the place where the exception is raised now isn't correct.
Please be more verbose. It concerns GinacFunction
s 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
 Status changed from needs_work to needs_info
comment:30 Changed 5 years ago by
#16812 depends on this.
comment:31 Changed 5 years ago by
 Commit changed from 81fab64d599d6e3e0dc1d142b86998b04042562b to b838310bd8b22284f60fdeff03a1de7b5cd9e49c
comment:32 Changed 5 years ago by
 Milestone changed from sage6.5 to sage6.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
 Description modified (diff)
comment:35 Changed 4 years ago by
 Branch changed from u/rws/allow_algorithm_keyword_when_calling_builtinfunction to u/rws/175311
comment:36 Changed 3 years ago by
 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:
acedb88  17531: allow algorithm keyword when calling BuiltinFunction

comment:37 Changed 3 years ago by
 Commit changed from acedb884403e9363471d71132b68c38a3c874606 to ebac431f4024ec7b92a44138f9dbeb8c6cd2d6bf
Branch pushed to git repo; I updated commit sha1. New commits:
ebac431  Merge branch 'develop' into t/17531/175311

comment:38 Changed 3 years ago by
********************************************************************** 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.82316401210310853.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
 Commit changed from ebac431f4024ec7b92a44138f9dbeb8c6cd2d6bf to b7e88d4ec6f74aa43f9fb5c8d9c54218492cba35
Branch pushed to git repo; I updated commit sha1. New commits:
b7e88d4  add doctest tolerances

comment:40 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:41 Changed 2 years ago by
 Milestone changed from sage6.10 to sageduplicate/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
 Resolution set to wontfix
 Status changed from positive_review to closed
closing positively reviewed duplicates
Is there something missing in the example?