refactor!: back nanoevents.methods.vector with scikit-hep vector #991

lgray · 2024-01-14T20:11:49Z

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 previously r, this can certainly screw up math if you're not expecting it

Breaking interface differences (as you can see in tests):

we're now beholden to vector's much more strict typing of vectors you have to use 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 vs deltaphi and delta_r vs. deltaR so we can actually deprecate that in a reasonable way.

Another thing we need to address is where the functions nearest and metric_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.

nsmith-

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.

src/coffea/nanoevents/methods/vector.py

nsmith- · 2024-01-18T16:53:09Z

tests/test_nanoevents_vector.py

-        0.6078019961139605,
-    ]
+
+    computed_dphi = a.delta_phi(b).to_numpy()


Is the explicit cast required now?

see the comment on approx.

tests/test_nanoevents_vector.py

src/coffea/nanoevents/methods/vector.py

lgray · 2024-01-18T17:18:12Z

unfortunately @jrueb has left academia for industry, unless he wants to review out of the kindness of his heart.

lgray · 2024-01-18T23:30:17Z

I'm putting in a deprecation for ~6 months from now over in #997.

ikrommyd · 2024-01-21T18:06:34Z

@lgray Is the plan to keep the public delta_phi and delta_r functions as is? I suggest you do because there are some cases where a user defined "eta" instead of the .eta attribute is needed

lgray · 2024-01-21T18:11:43Z

In the first phase of this, yes we'll keep those functions for backwards compatibility.

In the long run, when we remove nanoevents.methods.vector, vector supplies deltaR and deltaphi and folks should migrate to use what is already supplied. You can see we now just use those functions within their underscore equivalents.

lgray · 2024-01-26T22:19:20Z

@nsmith- tests fail after appeasing linter :-(

nsmith- · 2024-01-26T22:20:20Z

Yes, I added tests and am now working to improve them

lgray · 2024-01-26T22:22:39Z

Ah, gotcha - yeah putting this together with scikit-hep/vector#413 I figured you may be messing around.

lgray · 2024-01-30T16:18:22Z

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.

* More tests * Don't modify the global awkward behavior

for more information, see https://pre-commit.ci

…ector fix: Revert "fix: updates from review"

fix: use the new type signature for copy_behaviors

lgray · 2024-08-04T15:47:54Z

@Saransh-cpp looks like a few things broke... there are missing behavior functions according to the tests.

Saransh-cpp · 2024-08-04T16:25:28Z

Weirdly, I am not able to reproduce any of the failures locally. I guess that's why I gave this PR a green flag in the last meeting.

Saransh-cpp · 2024-08-05T11:24:54Z

I am having no luck in reproducing these locally, even with new environments. @lgray could you or someone else check if this is reproducible locally?

The GH Actions error looks unrelated (something wrong with pytest's installation in main).

lgray · 2024-08-05T16:04:35Z

I'll give it a spin soon here to see if I can reproduce.

lgray · 2024-08-05T18:31:13Z

@Saransh-cpp so this appears to be some windows-specific issues with paths in new images github produced.

There's still another error, and we'll see if that shows up without the changes in this PR.

lgray · 2024-08-05T18:50:13Z

@Saransh-cpp unfortunately it appears to be something with this PR. The tests with just awkward 2.6.7 over in #1149 are passing fine.

lgray · 2024-08-06T14:05:34Z

@Saransh-cpp any luck?

Saransh-cpp · 2024-08-06T23:17:14Z

Hi @lgray, I tried the tests in a fresh environment as well as on another system (windows), but I really don't know why I am not seeing the errors locally. I am a bit clueless here.

lgray · 2024-08-08T15:57:32Z

@Saransh-cpp so I reproduced this on my laptop:

>>> events.GenPart.parent
Traceback (most recent call last):
  File "/Users/lgray/miniforge3/envs/coffea-dev/lib/python3.12/site-packages/dask_awkward/lib/core.py", line 1578, in __getattr__
    cls_method = getattr_static(self._meta, attr)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/lgray/miniforge3/envs/coffea-dev/lib/python3.12/inspect.py", line 1882, in getattr_static
    raise AttributeError(attr)
AttributeError: parent

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lgray/miniforge3/envs/coffea-dev/lib/python3.12/site-packages/dask_awkward/lib/core.py", line 1580, in __getattr__
    raise AttributeError(f"{attr} not in fields.")
AttributeError: parent not in fields.

>>> events.GenPart._meta
<PtEtaPhiMLorentzVectorArray-typetracer [...] type='## * var * GenParticle[...'>

So somehow we are resolving to the wrong type! It should be a GenParticleArray-typetracer.

@jpivarski

lgray · 2024-08-08T16:11:43Z

The layout seems to be correct though:

<ListOffsetArray len='##'>
    <offsets><Index dtype='int64' len='##'>
        [## ... ##]
    </Index></offsets>
    <content><RecordArray is_tuple='false' len='##'>
        <parameter name='__record__'>'GenParticle'</parameter>
        <parameter name='__doc__'>'interesting gen particles '</parameter>
        <parameter name='collection_name'>'GenPart'</parameter>

Saransh-cpp · 2024-08-08T16:23:55Z

Hi @lgray. I was able to reproduce this on the third system 😬

I think I know why this is happening. Working on it right now.

fix: copy_behaviors should be called before class definition

lgray · 2024-08-08T17:45:20Z

@nsmith- finally passing tests except for what looks like a spurious failure on arm linux. I've re-requested your review some time ago, but now it looks like you can properly do one. I think most of what you asked last time has been answered.

Saransh-cpp · 2024-08-08T17:58:20Z

Two minor missing suggestions from the review, which are more of discussion topics (maybe in tomorrow's meeting) -

Throw a useful error on implicit upcast if ProjectClass*D is not defined by a vector subclass scikit-hep/vector#488
fix: refine the implementation of copy_behaviors scikit-hep/awkward#3177 (comment)

nsmith-

Looks good!

nsmith- · 2024-08-08T21:14:37Z

tests/test_nanoevents_vector.py

@@ -519,22 +534,36 @@ def test_inherited_method_transpose(lcoord, threecoord, twocoord):
        )
    elif twocoord == "PolarTwoVector":
        c = ak.zip(
-            {"r": [-10.0, 13.0, 15.0], "phi": [1.22, -1.0, 1.0]},
+            {"rho": [-10.0, 13.0, 15.0], "phi": [1.22, -1.0, 1.0]},


This renaming of r to rho is something we'll need to remember to include in the release notes.
Perhaps a migration guide is in order? Are there any other user-facing changes we'll have to advertise?

This the only one that isn't resolved by reading vector's documentation.

I think this one will be a fairly low hit-rate, since most people would specify a polar two vector using pT and phi, which is the same in both cases.

lgray marked this pull request as draft January 14, 2024 20:12

lgray mentioned this pull request Jan 14, 2024

Use scikit-hep vector to as the backend for vector math in nanoevents #992

Closed

lgray force-pushed the use_scikithep_vector branch 3 times, most recently from 2ddea28 to 43e1f19 Compare January 16, 2024 23:13

nsmith- requested changes Jan 18, 2024

View reviewed changes

lgray force-pushed the use_scikithep_vector branch from fa9d4e5 to b8e16a4 Compare January 18, 2024 19:54

lgray mentioned this pull request Jan 18, 2024

feat: deprecate coffea.nanoevents.methods.vector #997

Merged

lgray force-pushed the use_scikithep_vector branch 4 times, most recently from 9ca78b9 to d7ad56c Compare January 20, 2024 03:56

lgray force-pushed the use_scikithep_vector branch 7 times, most recently from 4a7ef1e to 2374127 Compare January 26, 2024 22:18

lgray force-pushed the use_scikithep_vector branch 3 times, most recently from 8946d59 to bbcf978 Compare January 30, 2024 15:23

Saransh-cpp and others added 3 commits July 19, 2024 11:44

Add back useful changes

dc80e61

* More tests * Don't modify the global awkward behavior

[pre-commit.ci] auto fixes from pre-commit.com hooks

8349641

for more information, see https://pre-commit.ci

Indentation

fed625b

Saransh-cpp mentioned this pull request Jul 19, 2024

Throw a useful error on implicit upcast if ProjectClass*D is not defined by a vector subclass scikit-hep/vector#488

Open

lgray and others added 6 commits July 19, 2024 13:45

Merge pull request #1133 from Saransh-cpp/revert-1130-use_scikithep_v…

54cc334

…ector fix: Revert "fix: updates from review"

fix: use the new type signature for copy_behaviors

6e48c17

Update src/coffea/nanoevents/methods/nanoaod.py

01f1c04

Merge pull request #1136 from Saransh-cpp/new-copy_behaviors

801b775

fix: use the new type signature for copy_behaviors

Merge branch 'master' into use_scikithep_vector

73b25af

Merge branch 'master' into use_scikithep_vector

29fe03b

use python -m pytest (windows path change!?)

955f769

Merge branch 'master' into use_scikithep_vector

c5d1123

Saransh-cpp and others added 2 commits August 8, 2024 17:45

fix: copy_behaviors should be called before class definition

94210ef

Merge pull request #1152 from Saransh-cpp/fix-tests

eed1bbb

fix: copy_behaviors should be called before class definition

bump date of complete vector deprecation

72f117d

nsmith- approved these changes Aug 8, 2024

View reviewed changes

lgray merged commit 37dfe81 into master Aug 8, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: back nanoevents.methods.vector with scikit-hep vector #991

refactor!: back nanoevents.methods.vector with scikit-hep vector #991

lgray commented Jan 14, 2024 •

edited

Loading

nsmith- left a comment

nsmith- Jan 18, 2024

lgray Jan 18, 2024

lgray commented Jan 18, 2024

lgray commented Jan 18, 2024

ikrommyd commented Jan 21, 2024

lgray commented Jan 21, 2024 •

edited

Loading

lgray commented Jan 26, 2024

nsmith- commented Jan 26, 2024

lgray commented Jan 26, 2024

lgray commented Jan 30, 2024

lgray commented Aug 4, 2024

Saransh-cpp commented Aug 4, 2024

Saransh-cpp commented Aug 5, 2024 •

edited

Loading

lgray commented Aug 5, 2024

lgray commented Aug 5, 2024

lgray commented Aug 5, 2024

lgray commented Aug 6, 2024

Saransh-cpp commented Aug 6, 2024

lgray commented Aug 8, 2024 •

edited

Loading

lgray commented Aug 8, 2024

Saransh-cpp commented Aug 8, 2024

lgray commented Aug 8, 2024 •

edited

Loading

Saransh-cpp commented Aug 8, 2024

nsmith- left a comment

nsmith- Aug 8, 2024

lgray Aug 8, 2024

refactor!: back nanoevents.methods.vector with scikit-hep vector #991

refactor!: back nanoevents.methods.vector with scikit-hep vector #991

Conversation

lgray commented Jan 14, 2024 • edited Loading

nsmith- left a comment

Choose a reason for hiding this comment

nsmith- Jan 18, 2024

Choose a reason for hiding this comment

lgray Jan 18, 2024

Choose a reason for hiding this comment

lgray commented Jan 18, 2024

lgray commented Jan 18, 2024

ikrommyd commented Jan 21, 2024

lgray commented Jan 21, 2024 • edited Loading

lgray commented Jan 26, 2024

nsmith- commented Jan 26, 2024

lgray commented Jan 26, 2024

lgray commented Jan 30, 2024

lgray commented Aug 4, 2024

Saransh-cpp commented Aug 4, 2024

Saransh-cpp commented Aug 5, 2024 • edited Loading

lgray commented Aug 5, 2024

lgray commented Aug 5, 2024

lgray commented Aug 5, 2024

lgray commented Aug 6, 2024

Saransh-cpp commented Aug 6, 2024

lgray commented Aug 8, 2024 • edited Loading

lgray commented Aug 8, 2024

Saransh-cpp commented Aug 8, 2024

lgray commented Aug 8, 2024 • edited Loading

Saransh-cpp commented Aug 8, 2024

nsmith- left a comment

Choose a reason for hiding this comment

nsmith- Aug 8, 2024

Choose a reason for hiding this comment

lgray Aug 8, 2024

Choose a reason for hiding this comment

lgray commented Jan 14, 2024 •

edited

Loading

lgray commented Jan 21, 2024 •

edited

Loading

Saransh-cpp commented Aug 5, 2024 •

edited

Loading

lgray commented Aug 8, 2024 •

edited

Loading

lgray commented Aug 8, 2024 •

edited

Loading