- Notifications
You must be signed in to change notification settings - Fork 149
Add Ops for Gaussian Hypergeometric Function, Pochhammer Symbol, and Factorials #90
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
Conversation
| @ColtAllen Do you mind squashing your commits into: 1) Add factorial and poch helpers and 2) Add Hyp2F1 and derivatives? |
14273e9 to 823a702 Compare
Commits are squashed, but I edited the wrong commit message; sorry about that. |
| You can edit the commit message with interactive rebase (backup your branch first if it's your first time) |
823a702 to 32b922a Compare | @ColtAllen Can you run pre-commit on this? |
| Hey @twiecki, After a conversation with @ricardoV94, we decided the next step(s) are for the |
12985af to 0b861d3 Compare Codecov Report
Additional details and impacted files@@ Coverage Diff @@ ## main #90 +/- ## ======================================= Coverage 79.95% 79.95% ======================================= Files 170 170 Lines 44856 44950 +94 Branches 9498 9510 +12 ======================================= + Hits 35863 35942 +79 - Misses 6780 6790 +10 - Partials 2213 2218 +5
|
| @OriolAbril does the error in RTD make any sense to you? |
| Doesn't mean anything to me, I'd need to try and reproduce it locally to get a better idea :/. Has nothing similar ever happened outside rtd? |
a3dd6b2 to cab2655 Compare | @ColtAllen I've finally come around this one. Do you want to have a look? |
cab2655 to 0778e10 Compare | def check_2f1_converges(a, b, c, z) -> bool: | ||
| num_terms = 0 | ||
| is_polynomial = False | ||
| | ||
| def is_nonpositive_integer(x): | ||
| return x <= 0 and x.is_integer() | ||
| | ||
| if is_nonpositive_integer(a) and abs(a) >= num_terms: | ||
| is_polynomial = True | ||
| num_terms = int(np.floor(abs(a))) | ||
| if is_nonpositive_integer(b) and abs(b) >= num_terms: | ||
| is_polynomial = True | ||
| num_terms = int(np.floor(abs(b))) | ||
| | ||
| is_undefined = is_nonpositive_integer(c) and abs(c) <= num_terms | ||
| | ||
| return not is_undefined and ( | ||
| is_polynomial or np.abs(z) < 1 or (np.abs(z) == 1 and c > (a + b)) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just idle curiosity, but what is the significance of the num_terms variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, when the series has a negative integer in the denominator it will eventually have a divide by zero and blow up... unless it has a negative integer in the numerator that will arrive at zero first, truncating the series before that happens
Nice! We discussed using Stan, but it looks like you were able to get this running with Also, thinking long-term, do you think this is good to go for the conceivable future, or are there known bottlenecks worth revisiting later as additional backends are adopted and improvements made elsewhere in |
| I didn't think of using Stan directly just adapting their implementation. Using numpy is certainly not the most efficient thing we can do. The broader issue is discussed here: #83 I want to try to take a stab at it in the following days, but for now I think numpy will suffice. Depending on the range of values it can actually converge pretty quickly (~10 iterations). Actually I wanted to ask if you could try it in the model that motivated the issue and see how it fares. |
| The rtd issue doesn't seem to happen in other PRs :/. |
| rtd build should get fixed by a rebase @ricardoV94 |
Co-authored-by: ColtAllen <10178857+coltallen@users.noreply.github.com>
0778e10 to 10d4d94 Compare
ricardoV94 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-approval warning :)
This PR is a continuation of #87 in a new fork and adds
Ops for the Gaussian Hypergeometric Function, Pochhammer Symbol, and Factorials ashyp2f1,poch, andfactorial, respectively.hyp2f1involves an infinite summation and uses ascipyimplementation for this reason, but oncescanis performant enough to assume this task,hyp2f1can be rewritten in terms ofpochandfactorialin a future PR.The only test failing is
test_gradforhyp2f1. It seems to be a data type issue: