Opened 10 years ago

Closed 3 years ago

Last modified 4 months ago

#2516 closed enhancement (fixed)

generalized hypergeometric functions should be implemented

Reported by: ddrake Owned by: cwitty
Priority: major Milestone: sage-6.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 vbraun)

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)

hyper.patch (40.1 KB) - added by fredrik.johansson 7 years ago.
tentative implementation of generalized hypergeometric functions
hyper3.patch (37.1 KB) - added by eviatarbach 4 years ago.
trac_2516.patch (42.0 KB) - added by eviatarbach 4 years ago.
trac_2516_2.patch (41.7 KB) - added by eviatarbach 4 years ago.

Download all attachments as: .zip

Change History (83)

Changed 7 years ago by fredrik.johansson

tentative implementation of generalized hypergeometric functions

comment:1 Changed 7 years ago by fredrik.johansson

  • Authors set to fredrik.johansson
  • 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 always-unevaluated 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 closed-form 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 7 years ago by kcrisman

  • Cc kcrisman added

comment:3 Changed 7 years ago by kcrisman

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 6 years ago by ktkohl

  • Cc ktkohl added

comment:5 Changed 5 years ago by kcrisman

  • 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 5 years ago by kcrisman

  • Cc benjaminfjones dsm added

comment:7 Changed 5 years ago by dsm

Okay, I'm going to see what can be done to get this merged.

comment:8 Changed 5 years ago by dsm

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 half-functional 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 4 years ago by kcrisman

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 4 years ago by eviatarbach

  • Cc eviatarbach added

comment:11 Changed 4 years ago by mmezzarobba

  • Cc mmezzarobba added

comment:12 Changed 4 years ago by eviatarbach

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 4 years ago by eviatarbach

comment:13 Changed 4 years ago by eviatarbach

  • Dependencies set to 14780 9556 14802

comment:14 Changed 4 years ago by eviatarbach

  • Dependencies changed from 14780 9556 14802 to #14780 #9556 #14802

comment:15 Changed 4 years ago by eviatarbach

Two of the failing doctests are caused by #14858.

comment:16 Changed 4 years ago by eviatarbach

  • Dependencies changed from #14780 #9556 #14802 to #14858 #14780 #9556 #14802

comment:17 follow-up: Changed 4 years ago by eviatarbach

  • 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 4 years ago by eviatarbach

comment:18 Changed 4 years ago by eviatarbach

  • Description modified (diff)

comment:19 Changed 4 years ago by eviatarbach

  • Description modified (diff)

New patch fixes a circular import issue which caused a problem with numerical evaluation; see #13028.

Changed 4 years ago by eviatarbach

comment:20 Changed 4 years ago by eviatarbach

  • Description modified (diff)

comment:21 Changed 4 years ago by chapoton

  • Keywords hypergeometric added

comment:22 Changed 4 years ago by rws

  • Branch set to u/rws/generalized_hypergeometric_functions_should_be_implemented

comment:23 Changed 4 years ago by rws

  • Authors changed from fredrik.johansson to Fredrik Johansson, Burcin Erocal, Ralf Stephan
  • Commit set to 9553ba95b5a901428f1b191bb92232038b68c50e
  • Milestone changed from sage-wishlist to sage-6.2
  • Status changed from needs_info to needs_review

Rebased on 6.2.beta7. The branch includes the (already merged) #14802. I fixed some but not all doctests.


New commits:

7b9665914802
978f313Trac 2516: generalized hypergeometric functions
9553ba9Trac 2516: fix some doctests, typos

comment:24 Changed 4 years ago by rws

  • Status changed from needs_review to needs_work

comment:25 Changed 4 years ago by rws

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/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: Substitution using function-call 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)
<ipython-input-3-499d03e2f57c> 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 4 years ago by eviatarbach

  • Authors changed from Fredrik Johansson, Burcin Erocal, Ralf Stephan to Fredrik Johansson, Eviatar Bach, Ralf Stephan
  • Reviewers set to Ralf Stephan

Thanks for working on this!

I believe you set the incorrect authors though.

comment:27 follow-up: Changed 4 years ago by eviatarbach

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 4 years ago by rws

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.96046447754e-08
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 4 years ago by rws

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 4 years ago by rws

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 4 years ago by git

  • Commit changed from 9553ba95b5a901428f1b191bb92232038b68c50e to 5a415fb8ced74dbb579f3e18fff0e2605778e299

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

5a415fbTrac 2516: further doctest fixes

comment:32 Changed 4 years ago by rws

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 4 years ago by git

  • Commit changed from 5a415fb8ced74dbb579f3e18fff0e2605778e299 to ebe4c6a12c88cf6fdf0e7eb7b43bcfe05dcab8a4

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

ebe4c6a2516: fix doctests

comment:34 Changed 4 years ago by rws

  • 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 follow-ups: Changed 4 years ago by nbruin

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

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)

Last edited 4 years ago by nbruin (previous) (diff)

comment:36 follow-up: Changed 4 years ago by nbruin

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 ; follow-up: Changed 4 years ago by 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?

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 4 years ago by nbruin

  • Dependencies #14858 #14780 #9556 #14802 deleted

comment:39 in reply to: ↑ 37 Changed 4 years ago by nbruin

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

The issues around sr_to_max should be fixed (and that's straightforward).

comment:40 follow-up: Changed 4 years ago by nbruin

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 side-effect:

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 side-effect 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 4 years ago by nbruin

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

Last edited 4 years ago by nbruin (previous) (diff)

comment:42 in reply to: ↑ 40 Changed 4 years ago by rws

  • 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 hypergeometric-specific either.


New commits:

bc1d66etrac 2516: fix maxima_lib to properly translate hypergeometric

comment:43 follow-up: Changed 4 years ago by rws

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?

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

comment:44 in reply to: ↑ 43 Changed 4 years ago by nbruin

Replying to rws:

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?

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 side-effect 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 break-down 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 4 years ago by git

  • Commit changed from bc1d66efab841c4670bc9e07f6b5c0e4a580e0dd to dfcc9b05c5ba39e8b24ebb135c265d1814ff3910

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

dfcc9b0fix "SR tuple" to "maxima list" conversion

comment:46 Changed 4 years ago by rws

Please rebase/merge.

comment:47 Changed 4 years ago by git

  • Commit changed from dfcc9b05c5ba39e8b24ebb135c265d1814ff3910 to b514b8bdc5ab36feef0ba516d9053743e1d9b4c2

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

b514b8bMerge branch 'develop' into ticket/2516

comment:48 Changed 4 years ago by rws

  • Branch changed from u/nbruin/ticket/2516 to u/rws/ticket/2516

comment:49 in reply to: ↑ 35 Changed 4 years ago by rws

  • 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 round-trip)

I'm afraid offhand I have no idea what this is about. Will try.


New commits:

4b4ed75Merge branch 'develop' into t/2516/ticket/2516
0dbbacd2516: fix last doctests

comment:50 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by rws

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 ; follow-up: Changed 4 years ago by nbruin

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 4 years ago by git

  • Commit changed from 0dbbacd5af23a111706b660ec997c3b302a33525 to 0f5a4ff613ba4ae4cd29b73f6f16d61725d19f6b

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

934d0572516: remove hack
0f5a4ff2516: fix doctest

comment:53 in reply to: ↑ 51 Changed 4 years ago by rws

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

comment:54 Changed 3 years ago by nbruin

  • 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 3 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:56 Changed 3 years ago by chapoton

  • 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 3 years ago by git

  • Commit changed from 0f5a4ff613ba4ae4cd29b73f6f16d61725d19f6b to 9c6749db3c97ebf62556702cf8d942c8a0f07649

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

eeb273eMerge branch 'develop' into t/2516/ticket/2516
62f7faaMerge branch 'master' into t/2516/ticket/2516
9c6749ddoctest coverage

comment:58 Changed 3 years ago by rws

  • Status changed from needs_work to needs_review

comment:59 Changed 3 years ago by git

  • Commit changed from 9c6749db3c97ebf62556702cf8d942c8a0f07649 to 051edff9d24a9ca43c897112fc08dd6e0d322ebd

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

051edffMerge branch 'develop' into t/2516/ticket/2516

comment:60 Changed 3 years ago by git

  • Commit changed from 051edff9d24a9ca43c897112fc08dd6e0d322ebd to 17572324c63584343661ec964fc635351f7b7103

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

f96a1dfMerge branch 'develop' into t/2516/ticket/2516
17572322516: lazy_import the module

comment:61 Changed 3 years ago by rws

  • Keywords special added
  • Work issues coverage, doctest deleted

comment:62 Changed 3 years ago by rws

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 non-lazily at function level. Local import does not resolve the docbuild error.

So, back as global non-lazy import into functions/all.py?

comment:63 Changed 3 years ago by rws

Looks like there are circular imports in maxima_lib. But is it the cause?

comment:64 Changed 3 years ago by git

  • Commit changed from 17572324c63584343661ec964fc635351f7b7103 to d36389d37cefe44cf9e5e957f5ed1829645463fb

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

a3bc86c16520: fix whole file imports
d36389dMerge branch 'u/rws/circular_import_in_maxima_lib_py' of trac.sagemath.org:sage into t/2516/ticket/2516

comment:65 Changed 3 years ago by rws

  • 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 3 years ago by nbruin

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 3 years ago by rws

  • 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 3 years ago by rws

Now that the lazy import experiment was abandoned, is there anything amiss?

comment:69 Changed 3 years ago by vbraun

  • Description modified (diff)
  • Reviewers changed from Ralf Stephan, Nils Bruin to Ralf Stephan, Nils Bruin, Volker Braun

comment:70 Changed 3 years ago by vbraun

Looks good to me but merge conflict with #16007. Either merge that branch in or wait until the next beta.

comment:71 Changed 3 years ago by git

  • Commit changed from f96a1df816f3201822dbde0553b68b48403a0f90 to b2f8dc5a1c82312271f2e2361b142358a66d0965

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

65107d4Merge branch 'develop' into t/8734/ticket/8734-1
4e073836882: add rules for e, i, I
e5a53436882: do word search/replace for symtable keys
4c1b0eb6882: correction to previous commit
518de3e6882: add symable rules for e,i,I; fix maxima_var; add doctests
7a6696bMerge branch 'u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage' of trac.sagemath.org:sage into t/8734/ticket/8734-1
a8df107Merge branch 't/8734/ticket/8734-1' into t/16007/ticket/16007
9e66d16Merge commit 'a8df107e76527d83a87456520395ab85dbc44050' of trac.sagemath.org:sage into t/2516/public/hypergeometric
5ac82242516: fix doctests affected by 16007 merge
b2f8dc52516: this 16007-related doctest fail seems to be uncovered only by 2516, so fix it here

comment:72 follow-up: Changed 3 years ago by rws

Thanks to all other authors and reviewers.

comment:73 in reply to: ↑ 72 ; follow-up: Changed 3 years ago by kcrisman

Thanks to all other authors and reviewers.

Thanks for sticking with it to make this a reality!

Does this fix either of the two remaining examples at #9908 (see here)? No worries about putting them here, but if so then we can mark that as an easy-to-fix ticket.

comment:74 in reply to: ↑ 73 Changed 3 years ago by rws

Replying to kcrisman:

Does this fix either of the two remaining examples at #9908 (see here)? No worries about putting them here, but if so then we can mark that as an easy-to-fix ticket.

Answered there.

comment:75 Changed 3 years ago by rws

  • 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 3 years ago by vbraun

  • Branch changed from public/hypergeometric to b2f8dc5a1c82312271f2e2361b142358a66d0965
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:77 Changed 3 years ago by tscrim

  • Commit b2f8dc5a1c82312271f2e2361b142358a66d0965 deleted

See #16752 for a followup with some code/doc tweaks.

comment:78 Changed 4 months ago by jdemeyer

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 function-call 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 4 months ago by jdemeyer

Follow-up at #23159.

Note: See TracTickets for help on using tickets.