#2516 closed enhancement (fixed)
generalized hypergeometric functions should be implemented
Reported by:  ddrake  Owned by:  cwitty 

Priority:  major  Milestone:  sage6.3 
Component:  symbolics  Keywords:  hypergeometric, special 
Cc:  burcin, fstan, kcrisman, ktkohl, benjaminfjones, dsm, eviatarbach, mmezzarobba  Merged in:  
Authors:  Fredrik Johansson, Eviatar Bach, Ralf Stephan  Reviewers:  Ralf Stephan, Nils Bruin, Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  b2f8dc5 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Sage should have support for generalized hypergeometric functions. They are an important class of special functions. They show up everywhere from differential equations to binomial coefficient identities.
Attachments (4)
Change History (83)
Changed 9 years ago by
comment:1 Changed 9 years ago by
 Cc burcin fstan added
 Report Upstream set to N/A
 Status changed from new to needs_work
I've attached essentially the code I wrote during with Sage Days 24 with some additions. Features include:
 Representation of PFQs as symbolic functions (with automatic simplification) and in alwaysunevaluated form
 Numerical evaluation (wrapping mpmath)
 Evaluation in terms of elementary functions in some simple cases
Important missing features include:
 Rewriting symbolic sums as PFQs
 Evaluation in terms of polygamma functions
 Various transformations permitting closedform evaluation in special cases
The patch includes symbolic versions of Bessel functions, which can be expanded as PFQs. This obviously needs to be merged with the existing Bessel function classes.
The main remaining issue is to handle multivariate symbolic functions properly (the current approach in Function_PFQ is something of a hack). Also, in the present patch, there are two PFQ classes. This is partially due to limitations/awkwardness of the symbolic function class, but regardless of whether necessary, it may be desirable to have a separate "backend" representation for hypergeometric functions that doesn't need to be concerned with automatic evaluation, numerical conversions, etc. I'm not sure what's the best way to organize it all.
comment:2 Changed 8 years ago by
 Cc kcrisman added
comment:3 Changed 8 years ago by
See also #9908, which is at least related (we need to translate Maxima hg functions).
Also, I think that we already have a lot of Bessel functions implemented, though they are not yet symbolic.
comment:4 Changed 7 years ago by
 Cc ktkohl added
comment:5 Changed 7 years ago by
 Component changed from misc to symbolics
Changing component. But this is a really nice start, hopefully can be combined with some of the currently existing bessel stuff etc.
comment:6 Changed 7 years ago by
 Cc benjaminfjones dsm added
comment:7 Changed 7 years ago by
Okay, I'm going to see what can be done to get this merged.
comment:8 Changed 7 years ago by
Having played around with this some, I think it'll be pretty straightforward to get in.. *after* we have Burcin's ticket allowing numerical_approx to take more general kwargs. If it weren't for the need to maintain backward compatibility with the algorithm kwarg for the old bessel functions, life would be easier.
I have a halffunctional wrapper which monkeypatches things to sort of get them to work, but once that patch gets in it becomes much simpler. So I think this should stay in a holding pattern until we get that one in.
comment:9 Changed 6 years ago by
If it weren't for the need to maintain backward compatibility with the algorithm kwarg for the old bessel functions, life would be easier.
Since #4102 is nearly finished, and has at least sort of dealt with this issue, perhaps one can just ignore that part of the Bessel issue. Instead, one could just add the _expand_hyper
methods to the stuff there. I do like the idea here and it would be a shame for it to bit rot further...
comment:10 Changed 6 years ago by
 Cc eviatarbach added
comment:11 Changed 6 years ago by
 Cc mmezzarobba added
comment:12 Changed 6 years ago by
Here is an initial version of an updated patch. It needs some work because it's failing a few tests, but I just wanted to post for comments.
Changed 6 years ago by
comment:13 Changed 6 years ago by
 Dependencies set to 14780 9556 14802
comment:14 Changed 6 years ago by
 Dependencies changed from 14780 9556 14802 to #14780 #9556 #14802
comment:15 Changed 6 years ago by
Two of the failing doctests are caused by #14858.
comment:16 Changed 6 years ago by
 Dependencies changed from #14780 #9556 #14802 to #14858 #14780 #9556 #14802
comment:17 followup: ↓ 30 Changed 6 years ago by
 Status changed from needs_work to needs_info
Plotting is now working fine (with the approach from #14801, thanks Volker!), except for getting deprecation warnings about using unnamed function parameters, which I have not been able to debug. Does anyone know how to fix this?
Another strange doctest failure is the one in _fast_callable_
; when I run it from interactive mode I get {CommutativeRings.element_class}(v_0)
, but the doctest returns {Fields.element_class}(v_0)
. Why is that?
Other than that the patch should be ready for review.
Changed 6 years ago by
comment:18 Changed 6 years ago by
 Description modified (diff)
comment:19 Changed 6 years ago by
 Description modified (diff)
New patch fixes a circular import issue which caused a problem with numerical evaluation; see #13028.
Changed 6 years ago by
comment:20 Changed 6 years ago by
 Description modified (diff)
comment:21 Changed 5 years ago by
 Keywords hypergeometric added
comment:22 Changed 5 years ago by
 Branch set to u/rws/generalized_hypergeometric_functions_should_be_implemented
comment:23 Changed 5 years ago by
 Commit set to 9553ba95b5a901428f1b191bb92232038b68c50e
 Milestone changed from sagewishlist to sage6.2
 Status changed from needs_info to needs_review
comment:24 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:25 Changed 5 years ago by
Three of the remaining doctest errors come from:
sage: f=fast_callable(hypergeometric([x], [], 2),domain=CDF,expect_one_var=True) sage: f(8) /home/ralf/sage/local/lib/python2.7/sitepackages/IPython/core/interactiveshell.py:2834: DeprecationWarning: Substitution using functioncall syntax and unnamed arguments is deprecated and will be removed from a future release of Sage; you can use named arguments instead, like EXPR(x=..., y=...) See http://trac.sagemath.org/5930 for details. exec code_obj in self.user_global_ns, self.user_ns 1.0 sage: f(x=8)  TypeError Traceback (most recent call last) <ipythoninput3499d03e2f57c> in <module>() > 1 f(x=Integer(8)) TypeError: __call__() got an unexpected keyword argument 'x'
The deprecation warning is triggered in plot()
and complex_plot()
with hypergeometric argument. I'm unsure where exactly the problem is. At the least fast_callable()
should behave consistently when given a named parameter, and if there is a problem in hypergeometric
, fast_callable()
should give the right warning. So, maybe a separate ticket for this is necessary.
comment:26 Changed 5 years ago by
 Reviewers set to Ralf Stephan
Thanks for working on this!
I believe you set the incorrect authors though.
comment:27 followup: ↓ 28 Changed 5 years ago by
I got the plotting problem before too, see comment 17. It's something in the fast_callable
implementation if I recall correctly, but I couldn't figure out how to fix it at the time.
comment:28 in reply to: ↑ 27 Changed 5 years ago by
Replying to eviatarbach:
I believe you set the incorrect authors though.
Oops, sorry.
I got the plotting problem before too, see comment 17. It's something in the
fast_callable
implementation if I recall correctly, but I couldn't figure out how to fix it at the time.
exp_integral_e()
is a BuiltinFunction
too, and needs two parameters but
sage: f=fast_callable(exp_integral_e(x,1),domain=CDF,expect_one_var=True) sage: f(8) 0.0452114820619 sage: f=fast_callable(hypergeometric([x], [], y),domain=CDF,vars=[x,y]) sage: f(8,9) 5.96046447754e08 sage: f=fast_callable(hypergeometric([x], [], 2),domain=CDF,vars=[x]) sage: f(8) 1.0
so I changed
f = fast_callable(f, domain=CDF, expect_one_var=TRUE)
to
f = fast_callable(f, domain=CDF, vars=[x])
in complex_plot()
. Now the one var case works but not the two etc. So it seems the handling of expect_one_var
in fast_callable()
is wrong, as I already said.
comment:29 Changed 5 years ago by
Forget all what I wrote. The deprecation warning is only printed once per session, which makes my conclusions arbitrary.
comment:30 in reply to: ↑ 17 Changed 5 years ago by
Replying to eviatarbach:
It's something in the
fast_callable
implementation if I recall correctly, but I couldn't figure out how to fix it at the time.
What's certain is that Wrapper_cdf.__call__()
gets the same arguments as with other working examples but that SR._call_element_()
gets called where the deprecation warning happens while with exp_integral_e(x,1)
its _eval_()
is called directly.
Another strange doctest failure is the one in
_fast_callable_
; when I run it from interactive mode I get{CommutativeRings.element_class}(v_0)
, but the doctest returns{Fields.element_class}(v_0)
. Why is that?
Interestingly if use h=exp_integral_e(x,1)
in the same doctest, I get
sage: h._fast_callable_(etb) {exp_integral_e}(v_0, 1)
So why is it not hypergeometric(None, None, v_0)
or similar in the doctest?
comment:31 Changed 5 years ago by
 Commit changed from 9553ba95b5a901428f1b191bb92232038b68c50e to 5a415fb8ced74dbb579f3e18fff0e2605778e299
Branch pushed to git repo; I updated commit sha1. New commits:
5a415fb  Trac 2516: further doctest fixes

comment:32 Changed 5 years ago by
This fixes all except the one in _fast_callable_
and two others I haven't looked closely at. Specifically, I resolved the deprecation warning by excluding dynamic classes from this warning.
comment:33 Changed 5 years ago by
 Commit changed from 5a415fb8ced74dbb579f3e18fff0e2605778e299 to ebe4c6a12c88cf6fdf0e7eb7b43bcfe05dcab8a4
Branch pushed to git repo; I updated commit sha1. New commits:
ebe4c6a  2516: fix doctests

comment:34 Changed 5 years ago by
 Status changed from needs_work to needs_review
Fixed all doctests. Please review. I had to exclude the hypergeometric function from random symbolic tests as that code blindly applies real numbers as parameters, and I didn't want to add try: ... except ValueError: pass
there.
comment:35 followups: ↓ 49 ↓ 50 Changed 5 years ago by
Excellent work! I wasn't able to checkout the branch here for testing, but I noticed one thing for the maxima_lib interfacing:
 You are installing an extra rule in
mqapply_to_sage
, which I assume is correct. This rule registered in the special_max_to_sage dictionary: Great! that's what you're supposed to do  However, for conversion back, there should be a corresponding entry in special_sage_to_max as well, and you're not adding that. You'd have to test, but from the rule in the other direction, I'd guess it should be something like:
sage.functions.hypergeometric.hypergeometric : lambda A, B, X : [[mqapply],[[max_hyper, max_array],lisp_length(A)1,lisp_length(B)1],A,B,X]
where
lisp_length=EclObject('length')
If you don't put this rule in place, you'll find that things like "limit", "integral" and "sum" (which use sr_to_max
) will probably not work correctly with hypergeometric function. (in fact, I'm not so sure maxima has much support for them anyway, but at least we should make sure that the functions can survive a roundtrip)
Another bit: the addition to sr_to_max
+ elif op == tuple: + return maxima(expr.operands()).ecl()
should read
+ elif op == tuple: + return EclObject( ([mlist],(sr_to_max(op) for op in expr.operands())) )
if a "symbolic tuple" should indeed be translated into a maxima list.
(the branch attached to this ticket currently doesn't build to a usable executable for me. I'm having some libntl conflict)
comment:36 followup: ↓ 37 Changed 5 years ago by
I found this suspicious (it's in the implementation of the _fast_callable_
method
+ self.__name__ = self.__repr__() # was clobbered by category mechanics + return etb.call(self, *map(etb.var, etb._vars))
One shouldn't touch the __name__
attribute, and certainly not during arbitrary executing code! Is it clear what code depends on the __name__
being identical to __repr__
? Can we solve this issue (cheaply) during __init__
, or perhaps avoid the clobbering altogether?
comment:37 in reply to: ↑ 36 ; followup: ↓ 39 Changed 5 years ago by
Replying to nbruin:
One shouldn't touch the
__name__
attribute, and certainly not during arbitrary executing code!
Do you have a reference for that?
Is it clear what code depends on the
__name__
being identical to__repr__
?
It fixes the issue mentioned above where the doctest gets different output than interactive Sage. The first doctest in _fast_callable_
.
Can we solve this issue (cheaply) during
__init__
.
Nope it happens after __init__
or perhaps avoid the clobbering altogether?
It is a consequence of hypergeometric
being a SE with tuples as parameter. In theExpression
constructor the class gets changed using dynamic_class()
and this appears to be responsible.
Overall, all long tests pass and I see no reason to hold this old ticket any longer.
comment:38 Changed 5 years ago by
 Dependencies #14858 #14780 #9556 #14802 deleted
comment:39 in reply to: ↑ 37 Changed 5 years ago by
Replying to rws:
Replying to nbruin:
One shouldn't touch the
__name__
attribute, and certainly not during arbitrary executing code!Do you have a reference for that?
Well, you can look it up. Python uses __name__
for any "named" object (e.g., defined by a def
or a class
or modules). These things aren't supposed to change (they are used for introspection). The fact that it does (and that that matters!) points to either a flaw in the category system or in the way it is used here.
It is a consequence of
hypergeometric
being a SE with tuples as parameter. In theExpression
constructor the class gets changed usingdynamic_class()
and this appears to be responsible.
Overall, all long tests pass and I see no reason to hold this old ticket any longer.
The issues around sr_to_max
should be fixed (and that's straightforward).
comment:40 followup: ↓ 42 Changed 5 years ago by
OK, the problem is in ext.fast_callable.function_name
, which gets called by sage.ext.fast_callable.ExpressionCall.__repr__
. with as argument the "function" component of the ExpressionCall?. The problem is: an expression doesn't have a __name__
. So you end up finding a __name__
somewhere higher in the inheritance. You just end up putting an extraneous attribute on the expression. To illustrate the nasty sideeffect:
sage: h = hypergeometric([],[],x) sage: h.__name__ 'CommutativeRings.element_class' sage: from sage.ext.fast_callable import ExpressionTreeBuilder sage: etb = ExpressionTreeBuilder(vars=['x']) sage: _ = h._fast_callable_(etb) sage: h.__name__ 'hypergeometric((), (), x)'
As you see, _fast_callable_
now has a sideeffect on h
!
A better solution would be to adapt the routine ext.fast_callable.function_name
(or the way it gets used). It's clear you're now feeding it things it wasn't designed for.
comment:41 Changed 5 years ago by
 Branch changed from u/rws/generalized_hypergeometric_functions_should_be_implemented to u/nbruin/ticket/2516
 Modified changed from 04/14/14 06:34:41 to 04/14/14 06:34:41
This should at least solve the maxima_lib translation problems.
comment:42 in reply to: ↑ 40 Changed 5 years ago by
 Commit changed from ebe4c6a12c88cf6fdf0e7eb7b43bcfe05dcab8a4 to bc1d66efab841c4670bc9e07f6b5c0e4a580e0dd
Replying to nbruin:
A better solution would be to adapt the routine
ext.fast_callable.function_name
(or the way it gets used). It's clear you're now feeding it things it wasn't designed for.
Yes but how to distinguish such a 'dynamical' expression from Expression
? The type displayed (sage.functions.hypergeometric.Expression_with_dynamic_methods
) cannot be tested for directly. To do it right it should not be hypergeometricspecific either.
New commits:
bc1d66e  trac 2516: fix maxima_lib to properly translate hypergeometric

comment:43 followup: ↓ 44 Changed 5 years ago by
What about
+ if isinstance(fn, Expression) and str(type(fn)).find('Expression_with_dynamic_methods') >= 0: + return "{%r}" % fn
Alternatively this seems to work too:
+ if isinstance(type(h),sage.structure.dynamic_class.DynamicMetaclass) + return "{%r}" % fn
Wouldn't one of these be safe enough?
comment:44 in reply to: ↑ 43 Changed 5 years ago by
Replying to rws:
Alternatively this seems to work too:
+ if isinstance(type(h),sage.structure.dynamic_class.DynamicMetaclass) + return "{%r}" % fnWouldn't one of these be safe enough?
I think the distinction should be easier to make. I don't think __name__
is reasonable on any expression. Compare:
sage: h = hypergeometric([], [], x) sage: s = sin(x) sage: from sage.ext.fast_callable import ExpressionTreeBuilder sage: etb = ExpressionTreeBuilder(vars=['x']) sage: v = h._fast_callable_(etb) sage: w = s._fast_callable_(etb) sage: type(v.function()) <class 'sage.functions.hypergeometric.Expression_with_dynamic_methods'> sage: type(w.function()) <class 'sage.functions.trig.Function_sin'>
As you can see, with more traditional functions, there's a rather more dedicated object in the function slot, for which the __name__
can be expected to be quite descriptive. For an expression, the __name__
attribute, if there's something there at all (and that there is seems a sideeffect of the dynamic classes. Normally, __name__
doesn't descend to class instances), can't really be descriptive. Looking at how to tell them apart:
sage: type(v.function()).mro() [sage.functions.hypergeometric.Expression_with_dynamic_methods, sage.symbolic.expression.Expression, sage.structure.element.CommutativeRingElement, sage.structure.element.RingElement, sage.structure.element.ModuleElement, sage.structure.element.Element, sage.structure.sage_object.SageObject, object] sage: type(w.function()).mro() [sage.functions.trig.Function_sin, sage.symbolic.function.GinacFunction, sage.symbolic.function.BuiltinFunction, sage.symbolic.function.Function, sage.structure.sage_object.SageObject, object]
So my guess is that you only need isinstance(type(h), sage.symbolic.expression.Expression)
, no need to check for substrings. On the other hand, your sage.structure.dynamic_class.DynamicMetaclass
seems to work too, even though that class doesn't show up in the mro!
I think we're running into a breakdown of ducktyping here: the code as written previously assumed that if a __name__
attribute is present, then it's meaningful. This is apparently not true for some dynamic classes and makes me think that maybe this __name__
attribute shouldn't be provided (wherever that happens).
comment:45 Changed 5 years ago by
 Commit changed from bc1d66efab841c4670bc9e07f6b5c0e4a580e0dd to dfcc9b05c5ba39e8b24ebb135c265d1814ff3910
Branch pushed to git repo; I updated commit sha1. New commits:
dfcc9b0  fix "SR tuple" to "maxima list" conversion

comment:46 Changed 5 years ago by
Please rebase/merge.
comment:47 Changed 5 years ago by
 Commit changed from dfcc9b05c5ba39e8b24ebb135c265d1814ff3910 to b514b8bdc5ab36feef0ba516d9053743e1d9b4c2
Branch pushed to git repo; I updated commit sha1. New commits:
b514b8b  Merge branch 'develop' into ticket/2516

comment:48 Changed 5 years ago by
 Branch changed from u/nbruin/ticket/2516 to u/rws/ticket/2516
comment:49 in reply to: ↑ 35 Changed 5 years ago by
 Commit changed from b514b8bdc5ab36feef0ba516d9053743e1d9b4c2 to 0dbbacd5af23a111706b660ec997c3b302a33525
Rebased on 6.2.beta8. I also included the test for Expression
in fast_callable
.
Replying to nbruin:
 You are installing an extra rule in
mqapply_to_sage
, which I assume is correct. This rule registered in the special_max_to_sage dictionary: Great! that's what you're supposed to do However, for conversion back, there should be a corresponding entry in special_sage_to_max as well, and you're not adding that. You'd have to test, but from the rule in the other direction, I'd guess it should be something like:
sage.functions.hypergeometric.hypergeometric : lambda A, B, X : [[mqapply],[[max_hyper, max_array],lisp_length(A)1,lisp_length(B)1],A,B,X]where
lisp_length=EclObject('length')If you don't put this rule in place, you'll find that things like "limit", "integral" and "sum" (which use
sr_to_max
) will probably not work correctly with hypergeometric function. (in fact, I'm not so sure maxima has much support for them anyway, but at least we should make sure that the functions can survive a roundtrip)
I'm afraid offhand I have no idea what this is about. Will try.
New commits:
4b4ed75  Merge branch 'develop' into t/2516/ticket/2516

0dbbacd  2516: fix last doctests

comment:50 in reply to: ↑ 35 ; followup: ↓ 51 Changed 5 years ago by
Replying to nbruin:
sage.functions.hypergeometric.hypergeometric : lambda A, B, X : [[mqapply],[[max_hyper, max_array],lisp_length(A)1,lisp_length(B)1],A,B,X]
It's already included, as far as I can see.
Are there any issues left?
comment:51 in reply to: ↑ 50 ; followup: ↓ 53 Changed 5 years ago by
Replying to rws:
It's already included, as far as I can see.
Yep, I figured out later how to get your branch to compile, so I provided the change
Are there any issues left?
It should be safe to delete the line
self.__name__ = self.__repr__()
now.
I haven't looked into much of the details of this; I mainly checked to see that translation to/from maxima_lib was done properly (and I think it is now).
comment:52 Changed 5 years ago by
 Commit changed from 0dbbacd5af23a111706b660ec997c3b302a33525 to 0f5a4ff613ba4ae4cd29b73f6f16d61725d19f6b
comment:53 in reply to: ↑ 51 Changed 5 years ago by
Replying to nbruin:
It should be safe to delete the line
self.__name__ = self.__repr__()
now.
Thanks. I also noticed that only the check for DynamicMetaclass
will make all resp. doctests pass.
I haven't looked into much of the details of this; I mainly checked to see that translation to/from maxima_lib was done properly (and I think it is now).
Please add your name as reviewer anyway. If you cannot give positive I will then ask in sagedevel.
comment:54 Changed 5 years ago by
 Reviewers changed from Ralf Stephan to Ralf Stephan, Nils Bruin
I can't vouch for the mathematical correctness here, but the translation to/from maxima seems sound.
comment:55 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:56 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to coverage, doctest
Some of the new functions needs to be doctested
comment:57 Changed 5 years ago by
 Commit changed from 0f5a4ff613ba4ae4cd29b73f6f16d61725d19f6b to 9c6749db3c97ebf62556702cf8d942c8a0f07649
comment:58 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:59 Changed 5 years ago by
 Commit changed from 9c6749db3c97ebf62556702cf8d942c8a0f07649 to 051edff9d24a9ca43c897112fc08dd6e0d322ebd
Branch pushed to git repo; I updated commit sha1. New commits:
051edff  Merge branch 'develop' into t/2516/ticket/2516

comment:60 Changed 5 years ago by
 Commit changed from 051edff9d24a9ca43c897112fc08dd6e0d322ebd to 17572324c63584343661ec964fc635351f7b7103
comment:61 Changed 5 years ago by
 Keywords special added
 Work issues coverage, doctest deleted
comment:62 Changed 5 years ago by
The lazy import in the last commit causes this:
[calculus ] loading cross citations... [calculus ] /home/ralf/sage/src/doc/en/reference/calculus/sage/calculus/calculus.rst:11: WARNING: error while formatting signature for sage.calculus.calculus.symbolic_expression_from_maxima_string: 'module' object has no attribute 'hypergeometric' [categorie] reading sources... [ 76%] sage/categories/modules_with_basis Error building the documentation. Traceback (most recent call last): File "/home/ralf/sage/src/doc/common/builder.py", line 1490, in <module> getattr(get_builder(name), type)() File "/home/ralf/sage/src/doc/common/builder.py", line 291, in _wrapper getattr(get_builder(document), 'inventory')(*args, **kwds) File "/home/ralf/sage/src/doc/common/builder.py", line 502, in _wrapper x.get(99999) File "/home/ralf/sage/local/lib/python/multiprocessing/pool.py", line 558, in get raise self._value OSError: [calculus ] /home/ralf/sage/src/doc/en/reference/calculus/sage/calculus/calculus.rst:11: WARNING: error while formatting signature for sage.calculus.calculus.symbolic_expression_from_maxima_string: 'module' object has no attribute 'hypergeometric'
I have no idea why. There appears to be no circular import. It can also be triggered with
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string sage: symbolic_expression_from_maxima_string('hypergeometric')
but that specific instance goes away when I import nonlazily at function level. Local import does not resolve the docbuild error.
So, back as global nonlazy import into functions/all.py
?
comment:63 Changed 5 years ago by
Looks like there are circular imports in maxima_lib
. But is it the cause?
comment:64 Changed 5 years ago by
 Commit changed from 17572324c63584343661ec964fc635351f7b7103 to d36389d37cefe44cf9e5e957f5ed1829645463fb
comment:65 Changed 5 years ago by
 Status changed from needs_review to needs_work
Now that the lazy import no longer causes a docbuild error a doctest fail appears:
sage t long src/sage/functions/hypergeometric.py # 1 doctest failed
comment:66 Changed 5 years ago by
I noticed this:
+lazy_import("sage.functions.hypergeometric", "hypergeometric") ... special_sage_to_max={ + hypergeometric : lambda A, B, X : [[mqapply],[[max_hyper, max_array],lisp_length(A.cdr()),lisp_length(B.cdr())],A,B,X] }
This can't do what I think it is meant to do. Either the reference to "hypergeometric" as a dictionary key leads to resolving the lazy import (this is the most likely, because we'll need hypergeometric.__hash__()
to set the dict entry), in which case there's no point in doing a lazy import, or some lazy proxy makes it into the dictionary as a key, which is then out of reach of the lazy import resolver to substitute with the real thing, leaving the proxy (and its slowdown) in place indefinitely.
I suspect that just importing hypergeometric
normally will lead to acceptable performance.
comment:67 Changed 5 years ago by
 Branch changed from u/rws/ticket/2516 to public/hypergeometric
 Commit changed from d36389d37cefe44cf9e5e957f5ed1829645463fb to f96a1df816f3201822dbde0553b68b48403a0f90
 Description modified (diff)
 Status changed from needs_work to needs_review
Reset to before lazy import. Reviewers will just have to ignore the PluginFailed
from patchbot.
I'll grab the occasion and announce that I will not run after every upgrade merge fail from now, but simply upload a patch that reviewers/other authors should apply at merge conflict. If that patch goes stale as well, too bad.
comment:68 Changed 5 years ago by
Now that the lazy import experiment was abandoned, is there anything amiss?
comment:69 Changed 5 years ago by
 Description modified (diff)
 Reviewers changed from Ralf Stephan, Nils Bruin to Ralf Stephan, Nils Bruin, Volker Braun
comment:70 Changed 5 years ago by
Looks good to me but merge conflict with #16007. Either merge that branch in or wait until the next beta.
comment:71 Changed 5 years ago by
 Commit changed from f96a1df816f3201822dbde0553b68b48403a0f90 to b2f8dc5a1c82312271f2e2361b142358a66d0965
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
65107d4  Merge branch 'develop' into t/8734/ticket/87341

4e07383  6882: add rules for e, i, I

e5a5343  6882: do word search/replace for symtable keys

4c1b0eb  6882: correction to previous commit

518de3e  6882: add symable rules for e,i,I; fix maxima_var; add doctests

7a6696b  Merge branch 'u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage' of trac.sagemath.org:sage into t/8734/ticket/87341

a8df107  Merge branch 't/8734/ticket/87341' into t/16007/ticket/16007

9e66d16  Merge commit 'a8df107e76527d83a87456520395ab85dbc44050' of trac.sagemath.org:sage into t/2516/public/hypergeometric

5ac8224  2516: fix doctests affected by 16007 merge

b2f8dc5  2516: this 16007related doctest fail seems to be uncovered only by 2516, so fix it here

comment:72 followup: ↓ 73 Changed 5 years ago by
Thanks to all other authors and reviewers.
comment:73 in reply to: ↑ 72 ; followup: ↓ 74 Changed 5 years ago by
comment:74 in reply to: ↑ 73 Changed 5 years ago by
comment:75 Changed 5 years ago by
 Status changed from needs_review to positive_review
I understand Volker's final remark as 'conditionally positive', and now the condition is fulfilled.
comment:76 Changed 5 years ago by
 Branch changed from public/hypergeometric to b2f8dc5a1c82312271f2e2361b142358a66d0965
 Resolution set to fixed
 Status changed from positive_review to closed
comment:77 Changed 5 years ago by
 Commit b2f8dc5a1c82312271f2e2361b142358a66d0965 deleted
See #16752 for a followup with some code/doc tweaks.
comment:78 Changed 2 years ago by
Does anybody remember why this inspect.ismethod
condition was added here to the deprecation from #5930? I mean, either something is deprecated or not. It shouldn't depend on some very subtle condition like the type of the _fast_callable_
attribute.
import inspect if not hasattr(_the_element,'_fast_callable_') or not inspect.ismethod(_the_element._fast_callable_): # only warn if _the_element is not dynamic from sage.misc.superseded import deprecation deprecation(5930, "Substitution using functioncall syntax and unnamed arguments is deprecated and will be removed from a future release of Sage; you can use named arguments instead, like EXPR(x=..., y=...)")
comment:79 Changed 2 years ago by
Followup at #23159.
tentative implementation of generalized hypergeometric functions