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

BUG: special: fix algorithmic error in ratevl in cephes/polevl.h #20730

Merged
merged 1 commit into from
May 16, 2024

Conversation

ZhibingSun
Copy link
Contributor

@ZhibingSun ZhibingSun commented May 16, 2024

Reference issue

Closes #20697.

What does this implement/fix?

It fixed an algorithmic error in ratevl in cephes/polevl.h.

Additional information

@github-actions github-actions bot added scipy.special C/C++ Items related to the internal C/C++ code base labels May 16, 2024
@lucascolley lucascolley added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label May 16, 2024
@lucascolley lucascolley changed the title Issue #20697 resolved. BUG: special: fix algorithmic error in ratevl in cephes/polevl.h May 16, 2024
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZhibingSun

@tupui tupui merged commit 535cad9 into scipy:main May 16, 2024
30 of 32 checks passed
@tupui tupui added this to the 1.14.0 milestone May 16, 2024
@lucascolley
Copy link
Member

and congrats on your first contribution to SciPy!

@@ -115,6 +116,7 @@ namespace cephes {

/* Evaluate a rational function. See [1]. */

/* The function ratevl is only used once in cephes/lanczos.h. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to keep this comment here, but just want to point out that this may change in the future, especially as we work on unifying the cephes, amos, specfun, cdflib, faddeeva, etc. codebases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I just checked the code for $erf(x)$ and $erfc(x)$ in special/Faddeeva.cc, which I think have better performance than the ones in cephes/ndtr.h, so is it possible to improve the performance of the functions in cephes/ndtr.h in the future, i.e., $erf(x)$, $erfc(x)$, and $ndtr(x)$?.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually found that cephes has better accuracy for real valued ndtr, erf, and erfc. See #19492 (comment).

I just meant that it's not unlikely that ratevl will end up being used again in another function, and I want to make sure to keep in mind that if so, this comment will need to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: special: algorithmic Error in ratevl in cephes/polevl.h
4 participants