-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat: add public delta_r
and delta_phi
kernels for user-specified phis and etas
#936
Conversation
I'm not sure whether wrapping a function that calls a numba vectorized function with |
Ah, duh, we can just use np.hypot if that's all we're encoding here. I have no idea why this didn't occur to me earlier! You can feel free to compare their execution speed, but If we need it in awkward array so that we can do dak dispatching of the call we should put it there as well. |
i.e. delta_r = np.hypot(eta2 - eta1, delta_phi(phi2, phi1)), so we've implemented everything already. It's mostly then an issue of threading it all through dak/ak. I suppose one additional thing the numba kernel buys us is the removal of an extra loop on the particles for delta phi. |
The whole point is just to add a delta_r kernel that users can import and use with their own definitions of etas and phis just because all the vector methods assume that the etas and phis the user wants to use are described by the from coffea.nanoevents.methods import vector
vector.delta_r(myeta1, myphi1, myeta2, myphi2) That is if you agree of course |
_delta_r_kernel
delta_r
and delta_phi
kernels for user-specified phis and etas
I agree but I'm just thinking about if it makes sense in the long run and/or where it really belongs. I'll merge it for now but I'm stuck between thinking it's too little to stick in a library (it's a one-liner in numpy even without the delta-phi kernel), but it's also reasonably convenient to just have it. Similarly such a tool doesn't belong buried in |
Perhaps part of a vector library such as the scikit-hep one. Or maybe coffea's vector should be a bit less buried like |
Adding a
_delta_r_kernel
so that users can specify their own eta and phi and calculatedelta_r
.We can also make this kernel public interface.