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

RisingFactorial cannot do numerical (floating point) evaluations #14822

Closed
cbm755 opened this Issue Jun 22, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@cbm755
Contributor

cbm755 commented Jun 22, 2018

>>> x = Float(S(2)/3, 32)
>>> RisingFactorial(18, x)
RisingFactorial(18, 0.66666666666666666666666666666667)
>>> N(_)
RisingFactorial(18, 0.66666666666666666666666666666667)
>>> _.doit()
RisingFactorial(18, 0.66666666666666666666666666666667)

Expected result: something like 6.82615401311257 (but with 32 digits).


There is an mpmath.rf and it works fine. I think its just not "connected" to sympy, perhaps because the names rf and RisingFactorial are different.

So probably this is an easy fix: find out how other special functions talk to mpmath and make it work for RisingFactorial (and FallingFactorial while you're at it).

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 22, 2018

@cbm755 I would like to work on this.
Please provide me with a few hints as well.
(I'm a newbie so pls bear with me. :)

@cbm755

This comment has been minimized.

Contributor

cbm755 commented Jun 22, 2018

I already gave my suggestions about first steps... In a bit more detail:

  1. convince yourself that mpmath.rf does the right thing.
  2. Pick another function (factorial, besseli, beta, bernoulli, etc). Check if they work (they do) and if they call mpmath.something.
  3. Dig into code to see how they call mpmath.
  4. Dig into RisingFactorial to see what is different.

Please note: if I could really give step-by-step hints I would know how to fix it myself right? ;-)

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 22, 2018

@cbm755 I got into the code and it turns out that RisingFactorial is actually assigned to rf later-on.
So the issue is not because of that ( as you commented above )

There is an mpmath.rf and it works fine. I think its just not "connected" to sympy, perhaps because the names rf and RisingFactorial are different.

It also turns out that the issue is only occurring when there's a floating point number as an argument.

Also, other functions (factorial, besseli, beta, bernoulli, etc) aren't calling mpmath.something at all.(according to the code)
PS. Pls Correct me if i'm wrong here.

I'm still working on the floating point correction part.

convince yourself that mpmath.rf does the right thing.

this is correct. I did find that mpmath.rf is working correctly.
So yes, there is a "no-connection" link between mpmath and sympy somewhere.
I'll get to you as soon as I get it.

skirpichev added a commit to skirpichev/diofant that referenced this issue Jun 22, 2018

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 22, 2018

Almost done..
Waiting for completion of tests.

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 23, 2018

@cbm755
It's working now. :)
I'll submit a PR once I finish remodelling the assertions

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 23, 2018

@cbm755 @asmeurer
I have made a PR.
Please do check and provide me with feedback.
This is my First PR.
Great experience. :)

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 24, 2018

@cbm755 I am currently working on another solution which will fill the discrepancies in this one and also make the results symbolic.
I'll be submitting another PR soon.

@cbm755

This comment has been minimized.

Contributor

cbm755 commented Jun 24, 2018

You should use the same PR.

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 24, 2018

@cbm755 @asmeurer
Also,
once the code starts working,
I'm getting the following assertion error (which is no surprise)
assert rf(n, m).is_integer is True AssertionError
since the resultant will not necessarily be an integer.
So should I change the assertion ?
I mean modify the test file containing that (and a couple of other assertions) ?

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 24, 2018

@cbm755 @asmeurer
I have made another PR. (implementing changes a bit differently)
( @cbm755 Sorry I didn't read your message earlier, or I would've made changes in the same PR)
Pls do give me a feedback on the new PR. :)
Thanks.

@asmeurer

This comment has been minimized.

Member

asmeurer commented Jun 24, 2018

If mpmath already has a function for it then you should be able to just add it to the evalf_table.

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jun 25, 2018

@asmeurer where can i find the evalf table?

@jksuom

This comment has been minimized.

Member

jksuom commented Jun 25, 2018

sympy.core.evalf, in _create_evalf_table().

@avishrivastava11

This comment has been minimized.

Contributor

avishrivastava11 commented Jul 11, 2018

@asmeurer @jksuom

If mpmath already has a function for it then you should be able to just add it to the evalf_table.

When I add it to the evalf_table, would I also be required to add the method corresponding to rf and ff, (as done with bernoulli, there is a method evalf_bernoulli added) ?
Or should I just somehow direct it to the class RisingFactorial (and FallingFactorial) present in factorials.py ?

@ramvenkat98

This comment has been minimized.

Contributor

ramvenkat98 commented Jul 22, 2018

@cbm755 @asmeurer

Hi,

To fix this issue, I've added _eval_evalf methods to the RisingFactorial and FallingFactorial classes that compute the result using the rf and ff functions in mpmath. I added test cases and created a pull request #14955 referencing this issue.

Thank you!

EDIT:

I've updated the pull request, adding these functions to the evalf_table instead of creating separate _eval_evalf functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment