Add numpy windowing functions support (np.bartlett, np.hamming, np.blackman, np.hanning, np.kaiser)

[synapticarbors]

Adding support for np.bartlett to address missing numpy features mentioned in #4074.

I will also try to tackle all of the windowing functions since they are quite similar.

[synapticarbors]

@stuartarchibald I think this is ready for an initial review. I've added the implementations for all of the windowing functions, with associated tests and changes to the docs, but since this is my first code contribution to numba, any feedback would be most appreciated. They are all pretty straightforward translations of the numpy source code, except for np.kaiser, which I re-wrote to avoid unifying type errors that seemed baked into the numpy source.

[stuartarchibald]

Thanks for the PR @synapticarbors, I've reviewed the code, it largely looks good! Most of my comments are around creating consistent behaviours for Numba, particularly about ensuring type inference problems are caught early. There's also some refactoring I think may be possible which in the long term should lead to less code to maintain. Thanks again :)

[stuartarchibald]

Also, looks like there's some floating point problems on 32bit platforms https://travis-ci.org/numba/numba/jobs/530906188#L3993, perhaps check np.float_ usage and look at the type information for the functions (env var NUMBA_DEBUG_TYPEINFER=1).

[synapticarbors]

@stuartarchibald -- Thank you for the review. I have done the following:

• Added comment about numpy version and hash that code was translated from
• combined all implementations except np.kaiser into a generator.
• combined tests
• added integer typing check in implementation and corresponding test
• cleaned up spacing around operators and line length to conform to stylistic preference of numba
• replaced the code to trick numba into accepting an empty list with an empty tuple as suggested
• Removed first 0 arg from np.arange since it can be left out and taken to be implicit.

What still needs to be done:

• implement np.i0. I took a shot at this (left the code in the commit) based on np.sinc, but it is more complicated and I don't completely understand how to do this. Scalar arguments return an array and the function raises if it cannot safely cast the input array.

• I was not immediately able to figure out what was going on with the floating point comparison for np.kaiser. I need to look into this further.

[stuartarchibald]

@synapticarbors thanks for making those changes WRT review, on initial inspection the clean-up patch looks good.

With regards to np.i0, apologies, I should have mentioned that this function can just be an @overload too I think. What you have tried using in the patches above is a low level API, the method that used to be used for adding functionality to Numba, it's useful but somewhat challenging! @overload was invented as an extension API but it turned out to be so useful that we try and write as much of the Numba internals with it as possible!

I think you should be able to do:

@overload(np.i0)
def np_i0(x):
  # you may need to dispatch to different impls based on type, this is fine...
  # define a few impls here and write branches in the typing scope as needed
  if isinstance(x, some_type):
    def impl(x):
      # the impl that was derived from cephes as per NumPy for "some_type"
    return the_result
  if isinstance(x, some_other_type):
    def impl(x):
      # the impl that was derived from cephes as per NumPy for "some_other_type"
    return the_result
  return impl

Once this is working we can take a look at the 32bit floating point issue with source code that is less challenging to manipulate!

Hope this helps, and thanks for your contributions :)

[synapticarbors]

@stuartarchibald it looks like np.i0 uses x.squeeze(), which does not appear to be supported in nopython mode. Is there a sensible workaround for this? For the use of np.i0 in np.kaiser, there's no need to worry about squeezing the array since it's always 1d, but obviously to provide a version of np.i0 for general use, this behavior is important.

Should we punt for now on np.i0 until np.squeeze has been implemented in nopython mode?

[stuartarchibald]

@synapticarbors I expect type based dispatch can be used to emulate squeeze, however, in the interests of getting this PR merged I agree with punting it for now.

[stuartarchibald]

@synapticarbors thanks for the update! I think this is just failing due to issues on 32bit platforms now, are you happy to look at this (if not, it's no problem, the Numba farm has all these combinations of machines set up!)?

[synapticarbors]

@stuartarchibald I set up a Linux VM last night to run a 32-bit version, since I'm on MacOS and it didn't look like one was available for it natively. I'm hoping I can get that configured today and reproduce the failed tests. If not, is there a recommended way to test 32-bit on MacOS? I looked briefly at Docker, but I'm not that familiar with it so wasn't sure what the best route was there.

[stuartarchibald]

@synapticarbors thanks for working on how to reproduce this! I'm not sure about OSX, perhaps VirtualBox? @seibert any ideas? It looks like it's just a floating point problem, but it would be nice to be able to determine if there's something fundamentally wrong where e.g. more extreme inputs would exacerbate the error, or if it is just floating point wobble.

[seibert]

If it is a precision issue that only manifests on 32-bit, then that sounds like some effect of using the x87 instructions (which have 80-bit precision for intermediate values until you flush to memory) vs. SSE instructions (which have 64-bit precision all the time).

[seibert]

To be more specific: If for some reason gcc and LLVM are sequencing things differently, and one is running out of x87 registers and having to evict values at a different point than the other, that could do this.

[seibert]

The difference is 1 ulp, so I think it would be reasonable to loosen up the precision requirement, at least on 32-bit Linux.

[stuartarchibald]

Good point, the error is at 1ULP. self.assertPreciseEqual() has a ulps kwarg, perhaps set that to 2 if numba.config.IS_32BITS is True?

[synapticarbors]

Thanks for the suggestion. I just pushed a commit to use a different ulps value for 32-bit. Hopefully that will work.

[synapticarbors]

Unfortunately still failing with ulps=2 for 32-bit:

0.036710892271286669 != 
0.036710892271286676

[seibert]

Ah, I see what is happening. assertPreciseEqual() is a little counterintuitive (though technically documented). The default precision (the prec argument) is exact, which ignores ulps and looks for exact match. What you actually need is prec="double", and then no ulps argument change is required. Example:

In [57]: t.assertPreciseEqual(0.036710892271286669, 0.036710892271286676, prec='exact', ulps=2)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-57-cb0c14bc8896> in <module>()
----> 1 t.assertPreciseEqual(0.036710892271286669, 0.036710892271286676, prec='exact', ulps=2)

/Users/sseibert/anaconda/lib/python2.7/site-packages/numba/tests/support.pyc in assertPreciseEqual(self, first, second, prec, ulps, msg, ignore_sign_on_zero, abs_tol)
    263             return
    264         # Decorate the failure message with more information
--> 265         self.fail("when comparing %s and %s: %s" % (first, second, failure_msg))
    266
    267     def _assertPreciseEqual(self, first, second, prec='exact', ulps=1,

/Users/sseibert/anaconda/lib/python2.7/unittest/case.pyc in fail(self, msg)
    408     def fail(self, msg=None):
    409         """Fail immediately, with the given message."""
--> 410         raise self.failureException(msg)
    411
    412     def assertFalse(self, expr, msg=None):

AssertionError: when comparing 0.0367108922713 and 0.0367108922713: 0.03671089227128667 != 0.036710892271286676

In [58]: t.assertPreciseEqual(0.036710892271286669, 0.036710892271286676, prec='double')

In [59]:

[synapticarbors]

Thanks for the clarification. Should I be using exact in the other tests and double in the one that is causing issues, or should I be uniformly be using double for theses tests? I'm not sure what the standard is.

[stuartarchibald]

ah, thanks @seibert :) totally forgot about that!

[seibert]

Exact is nice and unambiguous, but I think unrealistic for some floating point algorithms, especially when platforms have unpredictable roundoff error like x87 instructions on x86. I would say it is best to use double on 32-bit x86, and as needed on 64-bit if implementations need to diverge.

[synapticarbors]

So, a combination of prec="double" and increasing the ulps seems to have fixed the failures I was seeing before, but now there are new failures that I assuming are unrelated, and likely transient.

[seibert]

I have restarted the CI runs. I think you may have hit some temporary anaconda.org downtime.

[synapticarbors]

Thanks @seibert. It looks like everything ran without error this time. Is this ready to merge now? If so, can this be slipped into the 0.44.0 release?

[stuartarchibald]

@synapticarbors thanks for persisting with those floating point fixes. I just noticed one last thing to fix on the error handling tests that slipped through, and then I think this is good to go. Thanks again!

[stuartarchibald]

I think that this comment here #4076 (comment) needs further addressing at this location, whilst this asserts that it's a typing error, it doesn't assert it's the expected typing error. Also, when making this check, recall that the exception handling is in the typing domain, so variations in value have no impact. e.g. a foo(10.0) would raise the same error as foo(20.0), as 10.0 and 20.0 would both be seen as floats.

A typical method of doing this might be:

with self.assertRaises(TypingError) as raises:
  np_nbfunc('a', 10)
self.assertIn('M must be an integer', str(raises.exception))

then same for beta. Thanks.

[stuartarchibald]

Same comment from below RE testing exceptions applies here.

[synapticarbors]

Thanks for catching this. I think the latest commit fixes the oversight.

[synapticarbors]

One failure on Travis -- another network connectivity issue:

CondaHTTPError: HTTP 000 CONNECTION FAILED for url <https://conda.anaconda.org/numba/linux-64/repodata.json>

[stuartarchibald]

@synapticarbors This looks great, marking for merge. Thanks for all your efforts with this and congratulations on your first contribution to Numba! :)

[synapticarbors]

Thank you both for all of your help getting this one together. Hopefully I can work on a few more of these soon.

Also technically this is my first code contribution to numba. I had a doc edit that was merged a long time ago (bc16d77), but this definitely feels more satisfying :-)

[stuartarchibald]

@synapticarbors no problem at all, we look forward to receiving more PRs! Also, you are indeed correct, s/contribution/code contribution/:)