Guidance regarding replacing as_leading_term in place of extract_leading_order

[anutosh491]

Hello All ,as discussed in a recent PR , extract_leading_order of core/add.py is being deprecated because it is buggy and is used just once in the whole codebase(series module). So the line containing that is planned to be replaced by the as_leading_term method for better consistency. Now extract_leading_order returns the leading-term and the order of the leading-term as a tuple of tuple so recreating this using just as_leading_term wasn't a big deal

@@ -221,8 +223,15 @@ def __new__(cls, expr, *args, **kwargs):
             while old_expr != expr:
                 old_expr = expr
                 if expr.is_Add:
-                    lst = expr.extract_leading_order(args)
-                    expr = Add(*[f.expr for (e, f) in lst])
+                    from sympy import PoleError
+                    try:
+                        leading_terms = expr.as_leading_term(args)
+                        lst = []
+                        for term in leading_terms:  # for loop to deal with multivariate cases ,I also extended as_leading_term for those .                   
+                            lst.append((term, Order(term)))    # all multivariate tests are also working fine
+                        expr = Add(*[f.expr for (e, f) in lst])
+                    except PoleError:
+                        pass 

Just the git diff below would be enough to pass all related tests in order/limits/gruntz except those having exp(1/x) in them cause it would raise pole error hence the try -except block to deal with it ! But I am not sure what should the except block contains or how to deal with this rest all cases are working absolutely fine ! The problem is that

>>> (1/x**4 + exp(-x)).extract_leading_order(x)
((x**(-4), O(x**(-4))),) # this returns the correct answer rather than raising error like as_leading_term  . 

So I would be glad if anyone could guide me through the except block or rather what should come in place of pass .Thank You !

[jksuom]

There is no need for a separate expr.is_Add block. It should be possible to handle Add in the same way as the other expressions. To solve the 1/x**4 + exp(-x) case, it looks like exp(-1/x)._eval_as_leading_term(x, logx=..., cdir=d) should not raise but return 0 if re(d) >= 0

[anutosh491]

thanks for the help ! @jksuom
Yeah makes sense that the expr.is_Add block can be removed and the other elif block can be generalized as it also has an Add check , haven't yet explored removing that as of now ! Also will go through the above fix you suggested. Being my first PR in the series module any suggestions are highly welcome !

[0sidharth]

I think the support for multivariate expressions in as_leading_term has some scope of improvement currently. The current handling does not work for general cases. Also, refer to this comment in order.py -

sympy/sympy/series/order.py

Line 213 in 7d104b6

# XXX: better way? We need this expand() to

I think some changes like the following diff can be applied to improve the general handling of multivariate expressions.

https://gist.github.com/0sidharth/f9d6db5db6d7376dc3184ea8fb096dbb

This works for all test cases as far as I can see, with the following two changes (which seem not too big of an issue to me) -

In [1]: e = x*y*sin(x)*Integral(x, (x, 1, 2))
   ...: O(e)
Out[1]: O(x⋅y⋅sin(x); (x, y) → (0, 0))
# Previously
#  ⎛ 2                   ⎞
# O⎝x ⋅y; (x, y) → (0, 0)⎠

In [2]: Order(x + 2*y)
Out[2]: O(x + 2⋅y; (x, y) → (0, 0)) # Previously O(x + y; (x, y) → (0, 0))

[anutosh491]

Thanks a lot @0sidharth for the suggestion , seems to be working well. I was trying to do something similar but was trying to return a list having individual leading terms rather than a function which was creating some trouble for me , felt that it would be preferable as the expression name suggest it returns "terms" but this works .
And yeah the output for those two cases which differ don't seem to be wrong by any means , just differs from the previous output ! What are your thoughts on this @jksuom