Skip to content

[MRG] Cleaning of _log_logistic_sigmoid#13515

Merged
TomDLT merged 2 commits intoscikit-learn:masterfrom
NicolasHug:logistic_sigmoid_faster
Mar 25, 2019
Merged

[MRG] Cleaning of _log_logistic_sigmoid#13515
TomDLT merged 2 commits intoscikit-learn:masterfrom
NicolasHug:logistic_sigmoid_faster

Conversation

@NicolasHug
Copy link
Copy Markdown
Member

@NicolasHug NicolasHug commented Mar 25, 2019 •
edited
Loading

This PR slightly improves the code of _log_logistic_sigmoid:

  • loop variables are cdefed
  • helper function is inlined
  • use views instead of numpy arrays
  • indentation is corrected

The code is a tiny bit faster, though nothing to be excited about:

from sklearn.utils.extmath import log_logistic
import numpy as np
from time import time

X = np.random.normal(size=(1000, 2000))

n_runs = 100
times = []
for _ in range(n_runs):
    tic = time()
    log_logistic(X)
    times.append(time() - tic)

print(np.mean(times))
print(np.std(times))

This PR

0.08370588779449463
0.0011843339434451441

On master

0.08642879486083985
0.003170202198760095

ping @qinhanmin2014 since this somehow follows #13509

@NicolasHug NicolasHug changed the title [WIP] Cleaning of _log_logistic_sigmoid [MRG] Cleaning of _log_logistic_sigmoid Mar 25, 2019
jeremiedbb
jeremiedbb reviewed Mar 25, 2019
View reviewed changes
sklearn/utils/_logistic_sigmoid.pyx
cdef:
unsigned int i
unsigned int j

Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb Mar 25, 2019

Choose a reason for hiding this comment

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

There's no need to have an unsigned int here. i will never be bigger than n_samples which is an int.
Besides, it causes a warning of signed vs unsigned comparison. And I'm wondering if it doesn't cause a casting at each step of the loop. Are timings identical with ints ?

Copy link
Copy Markdown
Member Author

@NicolasHug NicolasHug Mar 25, 2019

Choose a reason for hiding this comment

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

Ok, I set everything to unsigned.

There's no cast (at least cython doesn't generate any cast). I didn't observe any speed difference

@TomDLT TomDLT merged commit ca35abe into scikit-learn:master Mar 25, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@NicolasHug
ENH Cleaning of _log_logistic_sigmoid (Cython) (scikit-learn#13515)
0c43fae
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "ENH Cleaning of _log_logistic_sigmoid (Cython) (scikit-learn#…
be911a5 
…13515)"

This reverts commit 0c43fae.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "ENH Cleaning of _log_logistic_sigmoid (Cython) (scikit-learn#…
17aef61 
…13515)"

This reverts commit 0c43fae.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@NicolasHug @koenvandevelde
ENH Cleaning of _log_logistic_sigmoid (Cython) (scikit-learn#13515)
63f936d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

+1 more reviewer

@TomDLT TomDLT TomDLT 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.

4 participants

@NicolasHug @agramfort @TomDLT @jeremiedbb