IntelLabs / numba

NumPy aware dynamic Python compiler using LLVM
http://numba.pydata.org/
BSD 2-Clause "Simplified" License
12 stars 2 forks source link

Add pyStencil, an AST based @stencil for use in testing. #36

Closed stuartarchibald closed 7 years ago

stuartarchibald commented 7 years ago

This adds pyStencil, a function that provides functionality similar to @stencil but in a much more reduced sense. It is used for testing the StencilFunc implementation and to aid reasoning about kernels and behaviours.

DrTodd13 commented 7 years ago

For test_basic24, what do you think the proper behavior should be? Seems like this should be defined as an error but the error message currently generated sure is a bad one. @stuartarchibald

stuartarchibald commented 7 years ago

@DrTodd13 RE: test_basic24 Whilst I can think of a variety of interpretations about how this could run, I'm struggling to think of a case where this would more useful than confusing. Therefore I think it ought to just raise an exception along the lines of "relative indexing must match the indexing of the input array, found 1 index expected 2", what do you think?

test_basic25 is of a similar ilk but with, I think, only one likely interpretation. However, I'm unsure it sensible to support it as I cannot think of a case where such a behaviour is useful and could not be achieved trivially with standard functionality. Can you think of one? Perhaps this is another case that should raise as it is quite possible that it is a result of user error/misinterpretation of behaviour?

DrTodd13 commented 7 years ago

Message for basic_test29 improved.

DrTodd13 commented 7 years ago

Copy propagation now working for test_basic31.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic24. I have it returning a better error message although the error is detected by type inference return an array instead of a scalar so I don't easily have enough information to say "you use a 1D index on a 2D array" kind of thing.

RE: test_basic25: this isn't testing a 1D index on 2d array. It is testing a constant kernel, which is kind of useless, and equivalent to full.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic32: I agree that in an ideal world, using np.int8(1) shouldn't fail but detecting that np.int8(1) is the equivalent to just "1" means that the tuple vars has to be traced back through the call, which has two parts, the "np.int" part and the "1" part and the former back through the corresponding getattr and a global corresponding to the "np" and "int" parts. So, it is doable but just mundane engineering and can be done in the future but I think we should hold off on this for now.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic35: Indeed, this case is expected to fail.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic38: This is basically the same as test 35. The cval type doesn't match the kernel return type so by the API it fails.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic45/46: We discussed this in the meeting to just detect and disallow assignments to arrays passed to stencil kernels. This check is now implemented and checked-in.

stuartarchibald commented 7 years ago

@DrTodd13 Thanks for looking at these.

RE: test_basic25: Yes it is entirely useless and an equivalent to full, I was a little surprised it worked though. The question I suppose is... should it work or should the impl spot there is no relative index and raise as it is most likely a mistake?

RE: test_basic32: Noted. It can indeed be done later, it is not a very likely use case.

RE: test_basic35/38: Noted. I agree. That is a documented item.

Re: test_basic45/46: Great thanks, that should prevent unusual behavior.

DrTodd13 commented 7 years ago

@stuartarchibald RE: test_basic26/27. Found the bug. Revealed some other issues to be fixed as well.

stuartarchibald commented 7 years ago

Closing this as it is replaced by #37