Blr pedestal and performance

[andLaing]

Refactorises the blr function removing the baseline calculation to be done outside the cythonised functions. In this way alternate functions can be tested.

Also changes the cythonised PMT loop for a standard map to improve performance. Per event time reduced from ~200 ms to ~25 ms on local machine.

[andLaing]

I've rebased and redone some performance checks both with NEW data (R7985) and with some NEXT100 simulations which I passed through buffy and diomira using np.repeat(NEWCOEFFICIENTS, 5) (3 ms buffers with the S2 in the centre) as the blr coefficients (we don't have values for NEXT100 as yet).

On my local machine the comparison with timeit gives the following time per event:

• NEW data event, MASTER: 238 ± 17.8 ms, PR: 25.1 ± 1.1 ms
• NEXT100 sim. event, MASTER: 2.07 ± 0.28 s, PR: 225 ± 2 ms

I ran a simple loop over 10 events for each case too and the improvement at the same order (NEW 2.19 s/331 ms, N100 20.5 s / 3.02 s). I've attached a couple of plots of the output for PMT sum. As expected (the algorithm itself isn't changed) both branches give the same result.

[jjgomezcadenas]

This PR is true to the advertising lines:

-Refactorises the blr function removing the baseline calculation to be done outside the cythonised functions. In this way alternate functions can be tested.

True, and tests are added

• Also changes the cythonised PMT loop for a standard map to improve performance. Per event time reduced from ~200 ms to ~25 ms on local machine.

Also true and demonstration added.

Code concise and to the point. Good job.

It does exactly what it claims

[jacg]

IIUC, moving some code from Cython to pure-python-using-numpy resulted in a speedup by a factor of 8.

Do you have some high-level explanation for why the Cython was that much slower, or some general lesson that can be learned from this?

[andLaing]

Very empirical opinion maybe but the cython function that's replaced used an astype on the pmt array argument (copy), then defined an output array for the individual pmts using zeros then did a loop over each PMT appending the deconvolution results to a list. By using the map we avoid the copies and it's optimised for that type of looping.

The baseline calculation didn't really make any difference to performance (maybe even slightly slower but within error) but gives the user more flexibility.

My take away, without really having used cython, is that you don't seem to gain anything unless you're doing something novel that can't use the internal optimisations of numpy or python itself.

[jacg]

Very empirical opinion maybe but the cython function that's replaced used an astype on the pmt array argument (copy), then defined an output array for the individual pmts using zeros then did a loop over each PMT appending the deconvolution results to a list. By using the map we avoid the copies and it's optimised for that type of looping.

Yes, in general you want to let Numpy do any looping over its arrays, rather than writing your own loops over numpy arrays.

(Well, I have seen Numba make hand-coded loops run faster than internal Numpy loops, but the gains were certainly not worth the hassle.)

My take away, without really having used cython, is that you don't seem to gain anything unless you're doing something novel that can't use the internal optimisations of numpy or python itself.

Indeed, this is the rule of thumb that everyone should follow (for Numpy; for plain Python it's a completely different story). If this is not obvious to everyone, then it's worth highlighting it here: Hand-coded Cython is unlikely to outperform things that Nupmy can do with internal loops!

So one might say that the problem with the original Cython function was that

• It tried to re-invent a wheel that was already available in Numpy, and

• as tends to be the case with re-invented wheels, it wasn't as good as the wheel which already existed.

Thanks for the summary ... and the PR!