Skip to content

[MRG+1] Add logsumexp and comb to utils.fixes (SciPy 0.19+) #9046

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
agramfort merged 1 commit into from
Jun 10, 2017
Merged

[MRG+1] Add logsumexp and comb to utils.fixes (SciPy 0.19+) #9046

agramfort merged 1 commit into from
Jun 10, 2017

Conversation

@naoyak
Copy link
Contributor

@naoyak naoyak commented Jun 7, 2017 •
edited
Loading

Add import checks for Add logsumexp and comb to utils.fixes, since they have been moved (with deprecation) from scipy.misc to scipy.special as of SciPy 0.19.

@naoyak naoyak changed the title Move logsumexp and comb to scipy.special [MRG] Move logsumexp and comb to scipy.special Jun 8, 2017
agramfort
agramfort reviewed Jun 8, 2017
View reviewed changes
sklearn/decomposition/online_lda.py Outdated
try: # SciPy >= 0.19
from scipy.special import logsumexp
except ImportError:
from scipy.misc import logsumexp
Copy link
Member

@agramfort agramfort Jun 8, 2017

Choose a reason for hiding this comment

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

i would uses fixes.py for this to avoid the try except in all our examples

you would write:

from sklearn.utils import logsumexp

agramfort
agramfort reviewed Jun 8, 2017
View reviewed changes
sklearn/utils/tests/test_random.py Outdated
try: # SciPy >= 0.19
from scipy.special import comb
except ImportError:
from scipy.misc import comb
Copy link
Member

@agramfort agramfort Jun 8, 2017

Choose a reason for hiding this comment

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

same remark here. Please put it in fixes.py

@naoyak naoyak changed the title [MRG] Move logsumexp and comb to scipy.special [MRG] Add logsumexp and comb to utils.fixes (SciPy 0.19+) Jun 8, 2017
@naoyak
Copy link
Contributor Author

naoyak commented Jun 8, 2017

@agramfort thanks, moved to utils.fixes

@lesteve
Copy link
Member

lesteve commented Jun 8, 2017

That means that we now have sklearn.utils.extmath.logsumexp and sklearn.fixes.logsumexp. This is a bit confusing and a bit sad at the same time ...

agramfort
agramfort reviewed Jun 8, 2017
View reviewed changes
sklearn/ensemble/tests/test_forest.py
from scipy.misc import comb
from scipy.sparse import csr_matrix
from scipy.sparse import csc_matrix
from scipy.sparse import coo_matrix
Copy link
Member

@agramfort agramfort Jun 8, 2017

Choose a reason for hiding this comment

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

we typically don't import everything in one line to avoid too many changed lines when adding or removing some imports.

please revert

@agramfort
Copy link
Member

agramfort commented Jun 8, 2017

agreed with @lesteve it should be in extmath. My bad.

also @naoyak please avoid cosmetic changes as it makes the diff bigger for nothing.

@lesteve
Copy link
Member

lesteve commented Jun 8, 2017 •
edited
Loading

agreed with @lesteve it should be in extmath. My bad.

I did not really say that. I just said that it was a bit unfortunate that:

  • we recently deprecated sklearn.utils.extmath.logsumexp. This completely makes sense since the not-overflowing functionality is now provided by all the scipy versions we support
  • need to add sklearn.utils.fixes.logsumexp as a compatibility layer between scipy.misc and scipy.special.

This leaves us with two logsumexp functions one in sklearn.utils.extmath the other one in sklearn.utils.fixes.logsumexp. This is slightly confusing but maybe we can live with this.

I don't really like the alternative which is to put logsumexp in sklearn.utils.extmath since it is just a compatibility layer and not a functionality that doesn't already exist in numpy or scipy.

@lesteve
Copy link
Member

lesteve commented Jun 8, 2017

Looks like Travis errors are genuine.

@naoyak
Copy link
Contributor Author

naoyak commented Jun 8, 2017 •
edited
Loading

@lesteve's point is well taken - it's weird to have the same logsumexp function present in both extmath and fixes.

We could just move the try-except block to the deprecated extmath version with a note to move the function to fixes when 0.21 rolls around. However from a maintenance standpoint it might be easier to keep both, and simply delete the extmath version about a year from now without having to update any imports elsewhere.

@naoyak naoyak closed this Jun 9, 2017
@naoyak naoyak reopened this Jun 9, 2017
lesteve
lesteve reviewed Jun 10, 2017
View reviewed changes
sklearn/utils/fixes.py
from scipy.sparse.linalg import lsqr as sparse_lsqr # noqa


try: # SciPy >= 0.19
Copy link
Member

@lesteve lesteve Jun 10, 2017

Choose a reason for hiding this comment

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

Should you not import from utils.fixes here?

Copy link
Contributor Author

@naoyak naoyak Jun 10, 2017

Choose a reason for hiding this comment

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

? this is in utils.fixes..

Copy link
Member

@lesteve lesteve Jun 10, 2017

Choose a reason for hiding this comment

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

Oops my bad, just ignore this ...

@lesteve
Copy link
Member

lesteve commented Jun 10, 2017

Out of interest do you close and reopen to re-trigger the CIs? You know you can push an empty commit or amend the commit and push -f to do this? The latter solution is one email instead of two ...

@naoyak
Copy link
Contributor Author

naoyak commented Jun 10, 2017

Oh sorry, I use both methods (often close-reopen when I'm not at my computer...) but I'll amend as much as possible to minimize spam, thanks for the heads up.

@lesteve
Copy link
Member

lesteve commented Jun 10, 2017

No worries, I also wanted to double-check with you that close + open was indeed restarting the CIs.

@lesteve lesteve changed the title [MRG] Add logsumexp and comb to utils.fixes (SciPy 0.19+) [MRG+1] Add logsumexp and comb to utils.fixes (SciPy 0.19+) Jun 10, 2017
@lesteve
Copy link
Member

lesteve commented Jun 10, 2017

LGTM

@agramfort agramfort merged commit 95aa295 into scikit-learn:master Jun 10, 2017
@agramfort
Copy link
Member

agramfort commented Jun 10, 2017

thanks @naoyak

@naoyak naoyak deleted the scipy-move branch June 10, 2017 21:05
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@naoyak @Sundrique
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
6a822ba
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@naoyak @dmohns
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
82a2085
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@naoyak @dmohns
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
d656d2d
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@naoyak @NelleV
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
16c3958
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@naoyak @paulha
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
eac87be
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@naoyak @AishwaryaRK
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
09c0bc5
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@naoyak @maskani-moh
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
ecd6882
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@naoyak
Add logsumexp and comb to utils.fixes (scikit-learn#9046)
bc32765
@jxu jxu mentioned this pull request Jun 24, 2019
Closed
@dexhunter dexhunter mentioned this pull request Nov 7, 2019
Merged
@ILikeTrains27 ILikeTrains27 mentioned this pull request Mar 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort left review comments

@lesteve lesteve lesteve left review comments

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

@naoyak @lesteve @agramfort