Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of appellf1 #14882

Merged
merged 9 commits into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@ashishkg0022
Copy link
Contributor

ashishkg0022 commented Jul 7, 2018

Implementing appellf1 to support rubi integration module.

@classmethod
def eval(cls, a, b1, b2, c, x, y):
if all(i.is_number for i in (a, b1, b2, c, x, y)):
return S(apl(a, b1, b2, c, x, y))

This comment has been minimized.

@asmeurer

asmeurer Jul 7, 2018

Member

I don't think this is the proper way to get automatic numeric evaluation.

This comment has been minimized.

@ashishkg0022

ashishkg0022 Jul 7, 2018

Author Contributor

can't we use mpmath's appellf1 to perform numeric evaluation ?

This comment has been minimized.

@Upabjojr

Upabjojr Jul 7, 2018

Contributor

Well, mpmath is for .evalf, not for .eval.

This comment has been minimized.

@asmeurer

asmeurer Jul 7, 2018

Member

It should already work automatically.

>>> class appellf1(Function):
...     nargs = 6
>>> appellf1(1., 2., 3., 4., 5., 6.)
-0.151473567018263

That's because Function subclasses automatically work with evalf if they use the same name as their mpmath counterpart, and _should_evalf (which is the proper way to do this) defaults to evalfing on float inputs.

elif args[0] == 6:
return (a*b2/c)*appellf1(a + 1, b1, b2 + 1, c + 1, x, y)
else:
return NotImplemented

This comment has been minimized.

@asmeurer

asmeurer Jul 7, 2018

Member

Does fdiff recognize NotImplemented? I don't recall any SymPy APIs using it.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 7, 2018

@raoulb you've done a lot of work with implementing special functions. Can you help review this?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 7, 2018

You could add some criteria to sort b1 and b2, in order to get a standardized order.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 7, 2018

For example:

if default_sort_key(b1) > default_sort_key(b2):
    b1, b2 = b2, b1
    x, y = y, x

This should solve the comparison problem.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 7, 2018

This should solve the comparison problem.

It also should canonicalize x and y if b1 and b2 are equal.

if default_sort_key(b1) > default_sort_key(b2):
b1, b2 = b2, b1
x, y = y, x
if b1 == b2:

This comment has been minimized.

@Upabjojr

Upabjojr Jul 8, 2018

Contributor

maybe elif

RisingFactorial(b2, n)/(factorial(m)*factorial(n)*RisingFactorial(c, m + n)), (m, 0, oo), (n, 0, oo)).rewrite(factorial)

def fdiff(self, argindex=5):
a, b1, b2, c, x, y = (i for i in self.args)

This comment has been minimized.

@Upabjojr

Upabjojr Jul 8, 2018

Contributor

no need for the generator, just = self.args.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 8, 2018

We should also check if the mpmath and the Mathematica implementations are identical.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 8, 2018

We should also check if the mpmath and the Mathematica implementations are identical.

I have checked some results, they are identical. http://docs.sympy.org/0.7.5/modules/mpmath/functions/hypergeometric.html#appellf1
I think exact implementation will vary.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 8, 2018

@asmeurer Why is the Travis failing?

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 8, 2018

You have unescaped backslashes in the docstring. You need to make it into a raw string (r""")

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 8, 2018

Why do you use __new__ instead of eval?

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 8, 2018

I just thought the reordering of b1 and b2 should be in __new__ .
That can be in eval too. Will this be a problem?

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 8, 2018

Should I use eval ?

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 9, 2018

They both work, but eval is more proper. Also, with eval, you don't need to define nargs.

ashishkg0022 added some commits Jul 9, 2018

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 9, 2018

Does this look fine ? I think this is enough for rubi.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 9, 2018

I was playing around with some values:

In [12]: appellf1(6,5,4,3,2,7).n()
---------------------------------------------------------------------------
ValueError: Analytic continuation not implemented

in Wolfram Mathematica:

In[7]:= AppellF1[6,5,4,3,2,7]

        1421
Out[7]= -----
        69984

I think this is a problem in mpmath. Or is it that the definitions are different?

elif argindex == 6:
return (a*b2/c)*appellf1(a + 1, b1, b2 + 1, c + 1, x, y)
elif argindex in (1, 2, 3, 4):
raise NotImplementedError("Unable to find derivative wrt to {}".format(self.args[argindex-1]))

This comment has been minimized.

@Upabjojr

Upabjojr Jul 9, 2018

Contributor

maybe this should return the unevaluated derivative:

return Derivative(self, self.args[argindex-1])
m, n = symbols('m n', integer=True)
assert appellf1(1 ,2 ,2, 1, 0.5, 0.6).rewrite(factorial) == \
Sum(0.5**m*0.6**n*factorial(m + 1)*factorial(n + 1)/(factorial(m)*\
factorial(n)), (m, 0, oo), (n, 0, oo))

This comment has been minimized.

@Upabjojr

Upabjojr Jul 9, 2018

Contributor

I was unable to get these expressions in Mathematica. Did you use Mathematica to verify these results?

This comment has been minimized.

@ashishkg0022

ashishkg0022 Jul 9, 2018

Author Contributor

RisingFactorial.rewrite(factorial) gives wrong result in sympy. I think we don't need this. Once this issue (#14871) is fixed, it can be added later.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 9, 2018

Or is it that the definitions are different?

No the definitions are same. Implementations are varying.
http://docs.sympy.org/0.7.5/modules/mpmath/functions/hypergeometric.html#appellf1
http://reference.wolfram.com/language/ref/AppellF1.html

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 9, 2018

do you think that it is worth to implement some specific values?
http://functions.wolfram.com/HypergeometricFunctions/AppellF1/03/ShowAll.html

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 9, 2018

the case x=0 and y=0 should probably be implemented.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 9, 2018

Ok, I am adding it.

@Upabjojr Upabjojr merged commit 7d274bc into sympy:master Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@parsoyaarihant parsoyaarihant added the GSoC label Jul 10, 2018

@ashishkg0022 ashishkg0022 deleted the ashishkg0022:appellf1 branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.