-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
ENH: fftpack: use float32 routines for float16 inputs. #6909
Conversation
@@ -128,12 +134,17 @@ def _asfarray(x): | |||
# 'dtype' attribute does not ensure that the | |||
# object is an ndarray (e.g. Series class | |||
# from the pandas library) | |||
if x.dtype.char is float16_code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings must not be compared with is
@@ -20,6 +20,12 @@ | |||
atexit.register(_fftpack.destroy_rfft_cache) | |||
del atexit | |||
|
|||
try: | |||
# no half-precision routines in fftpack. run float16 in single precision | |||
float16_code = numpy.dtype('float16').char |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
half-float exists since numpy 1.6.0. You can assume it is always available.
You don't need to declare float16_code
, just write dtype == np.half
below.
class TestFloat16FFT(TestCase): | ||
def setUp(self): | ||
self.cdt = np.complex64 | ||
self.rdt = np.float16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't inherit from _TestFFTBase
, there's no need for this.
Do you need to inherit from it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did originally, but changed it later and didn't remove the setUp. will remove it
return numpy.asarray(x, dtype=x.dtype) | ||
else: | ||
# We cannot use asfarray directly because it converts sequences of | ||
# complex to sequence of real | ||
ret = numpy.asarray(x) | ||
if ret.dtype.char not in numpy.typecodes["AllFloat"]: | ||
if ret.dtype.char is float16_code: | ||
return numpy.asarray(ret, dtype=numpy.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs Fortran-order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The other cases in _asfarray
seem to be C order
prior to this commit an error was raised for float16 input
e2839cd
to
82e06ce
Compare
I made the suggested changes and add a |
Can you add a comment in the affected |
@Eric89GXL : do you mean a docstring note for users or code comment for devs (or both)? |
also related: |
I just checked that the routines in |
I mean a docstring note. I would say leave the long double cases out for now, and just raise an error. If people really need those they can use another library as you suggest or cast down manually, which is more explicit anyway. |
LGTM. @pv feel free to have another look to see if you're happy with this new version. |
prior to this commit an error was raised for float16 input