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

Move logsumexp and pade from scipy.misc to scipy.special and scipy.interpolate #6759

Merged
merged 5 commits into from Nov 16, 2016

Conversation

@ev-br
Copy link
Member

ev-br commented Nov 4, 2016

This is supposed to tick a box in the roadmap:

scipy.misc will be removed as a public module. The functions in it can be moved to other modules:
...
comb, factorial, logsumexp, pade : special

comb and factorial have already been moved, this PR also moves logsumexp. EDIT: and pade.

One open question is whether from scipy.misc import factorial, comb, logsumexp should issue a DeprecationWarning pointing to their new location in scipy.special. At the moment, none of these does. If we want to actually remove scipy.misc, they probably should.

@person142

This comment has been minimized.

Copy link
Member

person142 commented Nov 4, 2016

Just wondering, but would it be better to put this stuff in separate files? (i.e. not in basic.py/test_basic.py.) Those files are IMO annoyingly large and unorganized.

@@ -618,6 +618,7 @@
round -- Round to nearest integer
xlogy -- Compute ``x*log(y)`` so that the result is 0 if ``x = 0``.
xlog1py -- Compute ``x*log1p(y)`` so that the result is 0 if ``x = 0``.
logsumexp -- Compute the log of the sum of exponentials of input elements.

This comment has been minimized.

Copy link
@person142

person142 Nov 4, 2016

Member

Annoying small nitpick--add extra spaces to restore the alignment? (Once you add logsumexp as an entry running cd tools && python refguide_summaries special will do all the adding of summaries/adjusting spacing automatically.)

This comment has been minimized.

Copy link
@ev-br

ev-br Nov 4, 2016

Author Member

Am I doing something wrong?

$ python refguide_summaries.py special
Traceback (most recent call last):
  File "refguide_summaries.py", line 112, in <module>
    main()
  File "refguide_summaries.py", line 79, in main
    doc = getattr(module, func).__doc__.split('\n')
AttributeError: 'module' object has no attribute 'roots_legendre'

This comment has been minimized.

Copy link
@person142

person142 Nov 4, 2016

Member

I'm at a loss... does python -c "from scipy.special import roots_legendre work?

This comment has been minimized.

Copy link
@ev-br

ev-br Nov 4, 2016

Author Member

No:

(scipy_py27)br@duneyrr:~/repos/scipy/tools$ python -c'from scipy.special import roots_legendre'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: cannot import name roots_legendre 

But:

$ python runtests.py --ipython
Building, see build.log...
Build OK
Python 2.7.11 (default, Dec 14 2015, 22:56:59) 
Type "copyright", "credits" or "license" for more information.

IPython 4.1.2 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: from scipy.special import roots_legendre

In [2]: roots_legendre?
Signature: roots_legendre(n, mu=False)
<snip>

This comment has been minimized.

Copy link
@ev-br

ev-br Nov 4, 2016

Author Member

Indeed, python setup.py install, and then it works. Thanks for the heads-up, I missed the introduction of this helper.

@pv

This comment has been minimized.

Copy link
Member

pv commented Nov 4, 2016

@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from c3e6a4d to 6607054 Nov 4, 2016
@ev-br

This comment has been minimized.

Copy link
Member Author

ev-br commented Nov 4, 2016

Comments addressed. Moved logsumexp to a separate file, squashed commits and force-pushed. A single commit is probably easier for git to follow so that the damage to git history is minimal.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Nov 5, 2016

These changes look good to me. In addition this would be useful:

  • a test to verify that misc.logsumexp exists and is special.logsumexp.
  • change the import in stats to import from special

Regarding a deprecation or future warning: that could be annoying for users. They'd have to write version checks like if scipy.__version > '0.19.0': from scipy.special .... <etc>. So what may be a better way is to migrate everything, give it another 2 releases or so, and then deprecate all of misc in one go.

@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from 6607054 to 2f82776 Nov 6, 2016
@ev-br ev-br changed the title Move logsumexp from scipy.misc to scipy.special Move logsumexp and pade from scipy.misc to scipy.special Nov 6, 2016
@ev-br

This comment has been minimized.

Copy link
Member Author

ev-br commented Nov 6, 2016

Comments addressed.
This PR also moves pade, for a good measure.

Examples
--------
>>> from scipy import misc

This comment has been minimized.

Copy link
@rgommers

rgommers Nov 7, 2016

Member

misc --> special

This comment has been minimized.

Copy link
@ev-br

ev-br Nov 7, 2016

Author Member

Nice catch! Fixed.

@person142

This comment has been minimized.

Copy link
Member

person142 commented Nov 7, 2016

Should pade really be in special? It seems like it would fit better in interpolate.

@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from cbab769 to fd8662d Nov 14, 2016
@ev-br ev-br added the needs-work label Nov 14, 2016
@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from fd8662d to f68d333 Nov 14, 2016
@ev-br ev-br changed the title Move logsumexp and pade from scipy.misc to scipy.special Move logsumexp and pade from scipy.misc to scipy.special and scipy.interpolate Nov 14, 2016
@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from f68d333 to 6c24738 Nov 14, 2016
@ev-br

This comment has been minimized.

Copy link
Member Author

ev-br commented Nov 14, 2016

Ok, redirected pade to interpolate, so that it's classed in "additional tools", along with lagrange and taylor_polynomial.

@ev-br ev-br removed the needs-work label Nov 14, 2016
@person142

This comment has been minimized.

Copy link
Member

person142 commented Nov 14, 2016

LGTM.


logsumexp(a, b=b)
def test_pade():
assert(pade is i_pade)

This comment has been minimized.

Copy link
@rgommers

rgommers Nov 15, 2016

Member

plain assert alert

This comment has been minimized.

Copy link
@ev-br

ev-br Nov 15, 2016

Author Member

Thanks, fixed

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Nov 15, 2016

LGTM too, modulo the assert. Let's leave it open for a little longer, merge if there are no objections to putting pade in interpolate.

scipy.misc.pade still exists for backwards compat
@ev-br ev-br force-pushed the ev-br:move_logsumexp branch from 6c24738 to 6206533 Nov 15, 2016
@rgommers rgommers added this to the 0.19.0 milestone Nov 16, 2016
@rgommers rgommers merged commit 29c715d into scipy:master Nov 16, 2016
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.86% (target 1.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Nov 16, 2016

Allright, in it goes. Thanks @ev-br !

@ev-br ev-br deleted the ev-br:move_logsumexp branch Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.