8000 Changed numpy to scipy since fftpack is in scipy by shanejohnpaul · Pull Request #3069 · gnuradio/gnuradio · GitHub
[go: up one dir, main page]
Skip to content

Changed numpy to scipy since fftpack is in scipy #3069

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
Closed
wants to merge 1 commit into from
Closed

Changed numpy to scipy since fftpack is in scipy #3069

wants to merge 1 commit into from

Conversation

shanejohnpaul
Copy link

fftpack is present in scipy not numpy

@mbr0wn
Copy link
Member
mbr0wn commented Jan 14, 2020

Did you get an error with the original code? from numpy.fft import fftpack works fine for me, and unlike scipy, numpy is an actual dependency of GNU Radio.

@shanejohnpaul
Copy link
Author
shanejohnpaul commented Jan 14, 2020 •
edited
Loading

Yeah. It said that it cannot import fftpack from numpy.fft. But with the scipy version, everything works fine. Does gnuradio require a specific numpy version?

Also, I couldn't find any documentation on numpy.fft.fftpack.

@velichkov
Copy link
Contributor

@mbr0wn,

It turns out the numpy's fftpack was replaced with pocketfft in numpy/numpy#11888, numpy/numpy@062dc8f so no fftpack in versions > v1.17.0rc1

The scipy is actually used in few places although there are no checks in cmake's files.

gnuradio/gr-filter/python/filter/design/filter_design.py

Line 53 in 96a5e82

from scipy import poly1d, signal

gnuradio/gr-filter/python/filter/design/fir_design.py

Line 23 in 96a5e82

import scipy

gnuradio/gr-trellis/python/trellis/fsm_utils.py

Line 33 in 96a5e82

import scipy.linalg

@mbr0wn
Copy link
Member
8000 mbr0wn commented Jan 14, 2020

Yeah scipy is one of those optional dependencies. Scripts using scipy are supposed to check for its existence and print a useful error on ImportError.

@velichkov
Copy link
Contributor

The same problem in gr-filter/python/filter/design/filter_design.py was fixed in #2755 by importing pocketfft in case importing of fftpack fails.

The fftpack is being used in:

  • gr-utils/python/utils/plot_fft_base.py
  • gr-dtv/examples/atsc_ctrlport_monitor.py
  • gr-digital/examples/example_timing.py

@devnulling
Copy link
Member

@shanejohnpaul Would you want to update this PR to behave the same as what the solution was in https://github.com/gnuradio/gnuradio/pull/2755/files ?

Also we will need a CLA from you, not specifically for this minor change, but it will streamline PRs that may have more LOC in the future. Please email bhilburn at gnuradio.org to get the CLA started.

@shanejohnpaul
Copy link
Author
shanejohnpaul commented Feb 16, 2020 •
edited
Loading

@devnulling Yes, it should behave the same. The fftpack is in scipy and not in numpy.fft, and that is all I have changed.

I'm new to PRs, so I have no idea about CLAs.

@drmpeg
Copy link
Member
drmpeg commented Feb 16, 2020

It should also be noted that the polar code QA tests fail during "make test" without scipy.

@marcusmueller
Copy link
Member

I'm really no fan of depending on scipy just because numpy's naming changed...

@michaelld michaelld mentioned this pull request Feb 27, 2020
Closed
@michaelld
Copy link
Contributor

I made an issue #3235 for this topic. Agreed we don't want to promote SciPy to being a configure requirement. There are better ways ...

@wbarnha wbarnha mentioned this pull request Mar 3, 2020
Closed
@nickoe nickoe added the fft label Mar 27, 2020
@mbr0wn
Copy link
Member
mbr0wn commented Mar 30, 2020

@shanejohnpaul based on our discussions in #3280 and #3290, it turns out this is the wrong fix. We need to call straight into numpy.fft. I'll close this PR, but still many thanks for submitting

@mbr0wn mbr0wn closed this Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dependencies fft
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@shanejohnpaul @mbr0wn @velichkov @devnulling @drmpeg @marcusmueller @michaelld @nickoe
0