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: Use npy_log1p where appropriate in random generation #18634

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

bashtage
Copy link
Contributor

@bashtage bashtage commented Mar 17, 2021

Extends #17020 and preserves legacy generators.

log1p was left in code where it makes no difference (e.g., npy_log1pf(-next_double(...)) which is always the same as log(1-next_double(...)) since it seems more consistent.

This reduces the number of cases when floating point precision makes the argument to `log` become `0`
@bashtage bashtage force-pushed the eric-wieser-log1p branch 3 times, most recently from ff4af80 to 826b0e0 Compare March 17, 2021 11:51
@bashtage bashtage changed the title BUG: Use npylog1p where appropriate in random generators BUG: Use npy_log1p where appropriate in random generation Mar 17, 2021
@bashtage
Copy link
Contributor Author

Only coverage failure, which I think is wrong here (but I can't see what codecov is seeing to check due to permissions).

Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Somehow codecoverage claims all the changes in distributions.c are not covered. Maybe because this is built as a library, and the optimization flags are somehow preventing the code coverage run?

In any case, this looks good to me.

numpy/random/_examples/cython/setup.py Outdated Show resolved Hide resolved
@bashtage bashtage force-pushed the eric-wieser-log1p branch 2 times, most recently from 368eee1 to e0317e5 Compare March 17, 2021 14:25
Use log1p(-x) instead of log(1 - x)
Seperate legacy version from current

closes numpy#17020
@mattip mattip merged commit aacb848 into numpy:main Mar 17, 2021
@mattip
Copy link
Member

mattip commented Mar 17, 2021

Thanks @bashtage

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.

None yet

3 participants