Skip to content
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

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

ikrommyd
Copy link
Contributor

@ikrommyd ikrommyd commented Nov 25, 2023

Adding a _delta_r_kernel so that users can specify their own eta and phi and calculate delta_r.
We can also make this kernel public interface.

@ikrommyd
Copy link
Contributor Author

I'm not sure whether wrapping a function that calls a numba vectorized function with numba.vectorize has any unexepected results but I didn't see anything whilst testing

@lgray
Copy link
Collaborator

lgray commented Nov 26, 2023

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 np.hypot should do it.

If we need it in awkward array so that we can do dak dispatching of the call we should put it there as well.

@lgray
Copy link
Collaborator

lgray commented Nov 26, 2023

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.

@ikrommyd
Copy link
Contributor Author

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 .eta and .phi fields. That is gonna be true like 99% of the time but it wasn't true in the case I asked about in mattermost.
That's why I'm thinking of making those kernels public API so that users can just do

from coffea.nanoevents.methods import vector
vector.delta_r(myeta1, myphi1, myeta2, myphi2)

That is if you agree of course

@ikrommyd ikrommyd changed the title feat: add _delta_r_kernel feat: add public delta_r and delta_phi kernels for user-specified phis and etas Nov 27, 2023
@lgray
Copy link
Collaborator

lgray commented Nov 27, 2023

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 nanoevents.methods.vector, analysis_tools probably makes more sense.

@lgray lgray merged commit 729e79d into CoffeaTeam:master Nov 27, 2023
15 checks passed
@ikrommyd
Copy link
Contributor Author

Perhaps part of a vector library such as the scikit-hep one. Or maybe coffea's vector should be a bit less buried like from coffea import vector instead of from coffea.nanoevents.methods import vector... idk

@ikrommyd ikrommyd deleted the feat-add-eta-phi-delta-r branch November 27, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants