8000 MNT log1p -> log in cython files by qinhanmin2014 · Pull Request #11852 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT log1p -> log in cython files #11852

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2018
Merged

MNT log1p -> log in cython files #11852

merged 1 commit into from
Aug 18, 2018

Conversation

@qinhanmin2014
Copy link
Member
@qinhanmin2014 qinhanmin2014 commented Aug 18, 2018

Revert the cython part of #11428
Will merge when green.

@rth
Copy link
Member
rth commented Aug 18, 2018

With respect to the error mentioned in #11848, I think the issue is that MSVC didn't include log1p in older versions (cf e.g. rust-lang/rust#29479). This is something we can revert once python 2 support is dropped (as Python 3 uses a newer version of MSVC on windows).

rth
rth approved these changes Aug 18, 2018
View reviewed changes
Copy link
Member
@rth rth left a comment

Choose a reason for hiding this comment

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

8000

Thanks !

@qinhanmin2014 qinhanmin2014 merged commit 5764baa into scikit-learn:master Aug 18, 2018
@qinhanmin2014 qinhanmin2014 deleted the appveyor-failure branch August 18, 2018 11:51
@qinhanmin2014 qinhanmin2014 mentioned this pull request Aug 18, 2018
Merged
@rth rth mentioned this pull request Aug 18, 2018
Merged
@jakirkham
Copy link
Contributor
jakirkham commented Aug 26, 2018

FWIW NumPy handles the problem by defining log1p when it is not available. Cython often exports NumPy C functions like this. So we could use those instead to get the best of both worlds (e.g. log1p when system provided and a custom C function when not).

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

Reviewers

1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@qinhanmin2014 @rth @jakirkham
0