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
refactor!: back nanoevents.methods.vector with scikit-hep vector #991
base: master
Are you sure you want to change the base?
Conversation
2ddea28
to
43e1f19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the only painful part is r
vs. rho
. Otherwise looks like it's drag and drop. @jrueb might want to review this as well.
0.6078019961139605, | ||
] | ||
|
||
computed_dphi = a.delta_phi(b).to_numpy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the explicit cast required now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the comment on approx.
unfortunately @jrueb has left academia for industry, unless he wants to review out of the kindness of his heart. |
fa9d4e5
to
b8e16a4
Compare
I'm putting in a deprecation for ~6 months from now over in #997. |
9ca78b9
to
d7ad56c
Compare
@lgray Is the plan to keep the public |
In the first phase of this, yes we'll keep those functions for backwards compatibility. In the long run, when we remove |
4a7ef1e
to
2374127
Compare
@nsmith- tests fail after appeasing linter :-( |
Yes, I added tests and am now working to improve them |
Ah, gotcha - yeah putting this together with scikit-hep/vector#413 I figured you may be messing around. |
8946d59
to
bbcf978
Compare
I think one place where we could benefit a lot from a thorough look-over by @Saransh-cpp is to make sure we're not unintentionally clobbering any of the vector interface with parts of nanoevents.vector, and likewise @nsmith- if there's anything we can do to make the delta between skh-vector-backed nanoevents.vector closer to the original implementation we should try to smooth all that out. |
Eventually we might also want to pawn these off on vector
63d2377
to
491507c
Compare
fix: depend on vector v1.3.1 + trim down coffea vectors
@nsmith- @Saransh-cpp from the looks of it we are now close to being able to merge this change. Please provide reasons not to do so yet, or any further tests that must be done before proceeding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me in this state!
This fails on a real user analysis with:
Please update and harden tests before we merge. This appears to be on the coffea side of things. |
fails in user analysis for missing overloads to candidate
Ah, this is going to be an issue. We have a class hierarchy that in principle would all need dispatch updates, so fixing it for |
So #1063 is just pushing the issue one level down. |
Oh yes, I missed that, thanks! I don't think there is a better way to do this? Should I add dispatch updates for |
More tests are failing, and I think there are more classes for which we'll have to do the same - |
@jpivarski is there a way to have awkward dispatch using the method resolution order of the mixin types, rather than having to register every subtype explicitly? |
The For ufunc and binary operator overloading, it's still the concrete classes that have to be named, but those a free functions, rather than methods and properties, so there isn't an easy way for inheritance to work. If you want to make @agoose77 and I brought this up several times—how to describe inheritance in ak.behavior[np.add, "Vector", "Vector"] = lambda v1, v2: v1.add(v2) would effectively do for left in [names of all subclasses of Vector]:
for right in [names of all subclasses of Vector]:
ak.behavior[np.add, left, right] = lambda v1, v2: v1.add(v2) The problem is that the interface of Having to build these things with for loops is cumbersome, but I'm afraid that having two parallel inheritance mechanisms would be worse in practice. |
Thank you for the explanation! I will add the loops for every vector class in coffee. Additionally, should coffea also have distinct and not the class names in the behavior dict? |
I'm not sure what you mean. Is it the distinction between putting behaviors in a global The fact that Coffea used But maybe some of Coffea's metadata handling costs come from passing Maybe I'm misunderstanding @Saransh-cpp's question, but either way, that might be something to consider: switching to |
Fixes #992
Right now this is breaking in terms of some low-level definitions of physics variables, in particular:
rho
is now always transverse, as opposed to it being previouslyr
, this can certainly screw up math if you're not expecting itBreaking interface differences (as you can see in tests):
to_VectorND()
to correctly compare vectors.But that doesn't seem to have too many side effects?
However, all the tests pass and the current implementation is backwards compatible in terms of
delta_phi
vsdeltaphi
anddelta_r
vs.deltaR
so we can actually deprecate that in a reasonable way.Another thing we need to address is where the functions
nearest
andmetric_table
should live, which are fairly often used but could be advertised better.@nsmith- This will require a thorough review as well as contribution from you, and we should discuss how we want to roll this out.