fix: vectors should be demoted to the lowest dimension

[Saransh-cpp]

Description

Fixes #412

Output of the snippet attached in the issue:

a+b
[{x: 0, y: -20, z: 10}, {x: 40, y: 0, z: 0}, {x: 0, y: 60, z: 30}]
{'__record__': 'Momentum3D'}
b+a
[{x: 0, y: -20, z: 10}, {x: 40, y: 0, z: 0}, {x: 0, y: 60, z: 30}]
{'__record__': 'Momentum3D'}

Checklist

• [ ] Have you followed the guidelines in our Contributing document?
• [ ] Have you checked to ensure there aren't any other open Pull Requests for the required change?
• [ ] Does your submission pass pre-commit? ($ pre-commit run --all-files or $ nox -s lint)
• [ ] Does your submission pass tests? ($ pytest or $ nox -s tests)
• [ ] Does the documentation build with your changes? ($ cd docs; make clean; make html or $ nox -s docs)
• [ ] Does your submission pass the doctests? ($ xdoctest ./src/vector or $ nox -s doctests)

Before Merging

• [ ] Summarize the commit messages into a brief review of the Pull request.

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (c4c90b9) 82.26% compared to head (1aaa2d1) 82.70%.

Files	Patch %	Lines
src/vector/backends/awkward.py	85.71%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #413 +/- ## ========================================== + Coverage 82.26% 82.70% +0.43% ========================================== Files 96 96 Lines 11429 11453 +24 ========================================== + Hits 9402 9472 +70 + Misses 2027 1981 -46 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Saransh-cpp]

@nsmith- could you check if this is working well? The new tests and the old tests are passing.

The code is a bit dirty; I will refactor it tomorrow and make this ready for review.

[Saransh-cpp]

I think this should be considered a breaking change in vector. @henryiii, how should the version be incremented once this is in? Should it go to v2.0.0 or just v1.2.0?

[nsmith-]

I checked out the branch, things look ok interactively, and the problematic coffea tests pass without the explicit cast now. (though some other tests fail, but I think this is on coffea side... checking)

[nsmith-]

Actually no, there is some issue now with the fact that we are subclassing the vector array method mixins:

>       boosted = a.boost(-a.boostvec)

tests/test_nanoevents_vector.py:313:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.env/lib/python3.10/site-packages/vector/_methods.py:3578: in boost
    return boost_beta3.dispatch(self, booster)
.env/lib/python3.10/site-packages/vector/_compute/lorentz/boost_beta3.py:393: in dispatch
    return _handler_of(v1, v2)._wrap_result(
.env/lib/python3.10/site-packages/vector/_methods.py:4192: in _handler_of
    handler = _demote_handler_vector(
.env/lib/python3.10/site-packages/vector/_methods.py:4144: in _demote_handler_vector
    if len({_handler_priority.index(obj.__module__) for obj in objects}) != 1:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

.0 = <tuple_iterator object at 0x12d673e20>

>   if len({_handler_priority.index(obj.__module__) for obj in objects}) != 1:
E   ValueError: 'coffea.nanoevents.methods.vector' is not in list

.env/lib/python3.10/site-packages/vector/_methods.py:4144: ValueError

(the .boostvec property is just calling .to_beta3())

[henryiii]

IMO, I'd be fine with calling this v1.2.0, since to me this is a bug fix - a rather impactful one, but still a bug fix. Before it would make a Momentum4D vector with only three dimensions, now the dimensions line up.

But also feel free to do what seems best to you.

[lgray]

Ah - this is great! Just seeing the PR now. :-)

[nsmith-]

There is still an issue with (a-b) vs. -(b-a):

import awkward as ak
from coffea.nanoevents.methods import vector

a = ak.zip(
    {
        "x": [10.0, 20.0, 30.0],
        "y": [-10.0, 20.0, 30.0],
        "z": [5.0, 10.0, 15.0],
        "t": [16.0, 31.0, 46.0],
    },
    with_name="LorentzVector",
    behavior=vector.behavior,
)
b = ak.zip(
    {
        "x": [-10.0, 20.0, -30.0],
        "y": [-10.0, -20.0, 30.0],
        "z": [5.0, -10.0, 15.0],
    },
    with_name="ThreeVector",
    behavior=vector.behavior,
)
c = ak.zip(
    {"rho": [-10.0, 13.0, 15.0], "phi": [1.22, -1.0, 1.0]},
    with_name="TwoVector",
    behavior=vector.behavior,
)

print("a-b")
print(a-b)
print(ak.parameters(a-b))
print("-(b-a)")
print(-(b-a))
print(ak.parameters(-(b-a)))

produces

a-b
[{x: 20, y: 0, z: 0, t: 16}, {x: 0, ...}, {x: 60, y: 0, z: 0, t: 46}]
{'__record__': 'LorentzVector'}
-(b-a)
[{x: 20, y: -0, z: -0}, {x: -0, y: 40, z: 20}, {x: 60, y: -0, z: -0}]
{'__record__': 'ThreeVector'}

They have a type matching their content, but the type demotion isn't working (there should be no t in the first case) ~Though a+b and b+a seem to be working OK.~ edit: no, a+b and b+a are both returning LorentzVectorArray type

[Saransh-cpp]

I think this is on the coffea side (on the use_scikithep_vector branch)? I might be wrong -

https://github.com/CoffeaTeam/coffea/blob/894496fc9fe8d3be79263bf7b03e72c18aaf13de/src/coffea/nanoevents/methods/vector.py#L507-L545

[nsmith-]

@Saransh-cpp you're right, it's to do with how we are building the subclasses' behavior dispatch tables. I've made a fix that now passes tests on the coffea PR with this PR.

[lgray]

Nice - Since we're deprecating nanoevents.vector in this next release of coffea. I think a reasonable timeline for getting a new version of vector with this is and merging the corresponding coffea PR is the beginning of March. Any faster than that and people may get spooked.

[lgray]

@Saransh-cpp concerning the version number for the next release. This isn't a major design-changing reimplementation of the package, but it does change behavior significantly, so 1.2.0 makes since IMO.

[Saransh-cpp]

Thanks for the reviews! Given that we don't have any release schedule for vector, I'll just merge this in and create a new release.