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

Fixed signal.get_window with unicode window name #3946

Merged
merged 1 commit into from Sep 2, 2014

Conversation

Projects
None yet
5 participants
@zeehio
Copy link
Contributor

zeehio commented Sep 1, 2014

This minor fix allows unicode objects to be used in scipy.signal.get_window.

This code does not fail anymore (python2):
from scipy.signal import get_window
get_window(u'blackman', 32) # This failed because u'blackman' is not a string on python2.

This code does not fail anymore (python2):
from future import unicode_literals
from scipy.signal import get_window
get_window('blackman', 32) # This failed because u'blackman' is unicode

@zeehio zeehio force-pushed the zeehio:master branch from 9db5771 to 371ee16 Sep 1, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 1, 2014

Coverage Status

Coverage remained the same when pulling 371ee16 on zeehio:master into 6761b2c on scipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 1, 2014

Coverage Status

Coverage increased (+0.0%) when pulling 371ee16 on zeehio:master into 6761b2c on scipy:master.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Sep 1, 2014

This fails with numpy 1.5.1:

 File "/home/travis/build/scipy/scipy/build/testenv/lib/python2.6/site-packages/scipy/signal/windows.py", line 7, in <module>
from numpy.compat.py3k import basestring
ImportError: cannot import name basestring

@zeehio zeehio force-pushed the zeehio:master branch from 371ee16 to 8e0f0ea Sep 2, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 2, 2014

Coverage Status

Coverage decreased (-0.0%) when pulling 8e0f0ea on zeehio:master into 6f9b0a7 on scipy:master.

@zeehio

This comment has been minimized.

Copy link
Contributor Author

zeehio commented Sep 2, 2014

@rgommers I am sorry. I did not account for all the combinations of python and numpy and I did not have travis set up. Now that all travis tests pass successfully. Thank you for taking a look into it.

@argriffing

This comment has been minimized.

Copy link
Contributor

argriffing commented Sep 2, 2014

I'm not too familiar with python 3, but scipy has a forked version of six.py. Would it make sense to use scipy.lib.six somehow?
https://github.com/scipy/scipy/blob/master/scipy/lib/six.py

@zeehio zeehio force-pushed the zeehio:master branch from 8e0f0ea to d120ea4 Sep 2, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 2, 2014

Coverage Status

Coverage increased (+0.0%) when pulling d120ea4 on zeehio:master into 6f9b0a7 on scipy:master.

@zeehio

This comment has been minimized.

Copy link
Contributor Author

zeehio commented Sep 2, 2014

I am not familiar enough with the scipy code, thanks @argriffing. Now the diff is much cleaner :-)

pv added a commit that referenced this pull request Sep 2, 2014

Merge pull request #3946 from zeehio/master
ENH: signal: fixed signal.get_window with unicode window name

@pv pv merged commit 70fa719 into scipy:master Sep 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@pv

This comment has been minimized.

Copy link
Member

pv commented Sep 2, 2014

Thanks, looks ok. Regression test would be useful, though.

@pv pv added this to the 0.15.0 milestone Sep 3, 2014

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