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

Fix docstrings reference for the betainc_grad function #412

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

amyoshino
Copy link
Member

Motivation for these changes

Currently, the betainc_grad() and gammainc_grad() functions have the same docstring:

Gradient of the regularized lower gamma function (P) wrt to the first
argument (k, a.k.a. alpha).

Adapted from STAN `grad_reg_lower_inc_gamma.hpp`

Reference: Gautschi, W. (1979). A computational procedure for incomplete gamma functions.
ACM Transactions on Mathematical Software (TOMS), 5(4), 466-481.

The fix attempts to clarify the function and indicates a probable resource that was utilized during the development of the function. I attempted to determine if it was indeed adapted from STAN, like in the case of 'Gradient of the regularized lower gamma function', but I couldn't be entirely certain. In any case, I've opened this PR to initiate a discussion on how to address this. Please let me know if I have referred to the incorrect resource.

Implementation details

...

Checklist

Major / Breaking Changes

  • ...

New features

  • ...

Bugfixes

  • ...

Documentation

  • ...

Maintenance

  • ...

@ricardoV94
Copy link
Member

Nice catch. IIRC this is actually not adapted from STAN. I'll try to retrieve the references

@ricardoV94
Copy link
Member

The docstring was correct prior to the refactor in 39d37df so we can recover it from there. Seems to have been:

    """
    Gradient of the regularized incomplete beta function wrt to the first
    argument (alpha) or the second argument (beta), depending on whether the
    fourth argument to betainc_der is `True` or `False`, respectively.

    Reference: Boik, R. J., & Robison-Cox, J. F. (1998). Derivatives of the incomplete beta function.
    Journal of Statistical Software, 3(1), 1-20.
    """

We should change the text to something like

    """
    Gradient of the regularized incomplete beta function wrt to the first
    argument `p` (aka alpha) or the second argument `q` (aka beta),
    depending on whether `wrtp` is true.

    Reference: Boik, R. J., & Robison-Cox, J. F. (1998). Derivatives of the incomplete beta function.
    Journal of Statistical Software, 3(1), 1-20.
    """

@amyoshino
Copy link
Member Author

Thanks for pointing out the references! I updated the code to reflect your suggestions. Let me know if I miss anything.

@ricardoV94 ricardoV94 merged commit 686811c into pymc-devs:main Aug 14, 2023
@ricardoV94
Copy link
Member

Thanks @amyoshino

@amyoshino amyoshino deleted the beta_inc_docstring branch August 14, 2023 17:25
@ricardoV94 ricardoV94 changed the title Fix docstrings reference for the betainc_grad function Fix docstrings reference for the betainc_grad function Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants