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: implement sum, count, and count_nonzero reductions #347

Merged
merged 45 commits into from
Jun 16, 2023

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented May 16, 2023

@jpivarski @Saransh-cpp this is a WIP initial stab at supporting sum, count, and count_nonzero reducers in vector across both NumPy and Awkward.1

My main query here is around API design; I note that @jpivarski and @henryiii designed the initial prototype, so you'll both likely have stronger feelings on how this deviates from the intended design. In particular, is it reasonable to fit reducers (in this case, only sum) into the compute model that is already established? From our type hints here, we don't explicitly encode any expectation that the vectors in compute functions are array like (or indeed satisfy any type checks), so I think this is fine.

Awkward support depends upon scikit-hep/awkward#2458.

Footnotes

  1. count is Awkward specific, however.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2023

Codecov Report

Patch coverage: 68.42% and project coverage change: -0.10 ⚠️

Comparison is base (8ca75f6) 83.01% compared to head (f964f97) 82.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
- Coverage   83.01%   82.91%   -0.10%     
==========================================
  Files          96       96              
  Lines       11372    11435      +63     
==========================================
+ Hits         9440     9481      +41     
- Misses       1932     1954      +22     
Impacted Files Coverage Δ
src/vector/backends/awkward.py 81.21% <51.35%> (-1.83%) ⬇️
src/vector/backends/numpy.py 81.50% <84.61%> (+0.37%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@agoose77 agoose77 requested a review from jpivarski May 17, 2023 23:23
@agoose77 agoose77 marked this pull request as ready for review May 17, 2023 23:23
@agoose77
Copy link
Contributor Author

Here's a rewrite that implements the reductions for each array backend. I need to cleanup the type hints, but it's mostly there.

@Saransh-cpp
Copy link
Member

This should also close #92 and maybe iris-hep/func_adl_uproot#98 🎉

PS: I had a very dirty somewhat working implementation for np.sum here - #92 (comment)

@Saransh-cpp Saransh-cpp linked an issue May 18, 2023 that may be closed by this pull request
@agoose77
Copy link
Contributor Author

This should also close #92 and maybe iris-hep/func_adl_uproot#98 🎉

PS: I had a very dirty somewhat working implementation for np.sum here - #92 (comment)

Oops, I missed this! Well, good to know it's needed!

@agoose77
Copy link
Contributor Author

agoose77 commented May 19, 2023

NB, this PR took a journey following feedback from Jim. In its current (final!) form, we don't worry with coordinate-systems; everything is moved into Cartesian, and remains in Cartesian.

@jpivarski
Copy link
Member

everything is moved into Cartesian, and remains in Catesian

Some (many?) of the ufunc-like functions do, too. This is not unique to reducers.

@agoose77 agoose77 marked this pull request as draft June 6, 2023 08:58
@agoose77 agoose77 marked this pull request as ready for review June 6, 2023 12:58
@agoose77 agoose77 requested a review from jpivarski June 10, 2023 15:07
@agoose77
Copy link
Contributor Author

This is ready to go!

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ready to go now because Awkward 2.2.3 is out. (The tests would have failed if they couldn't reach it.)

I did another scan through the diffs, and I'm reminded that this includes some bug-fixes as well as the new feature, but I guess that's alright. (There's assignment of __numba_typer__ and __numba_lower__ into Vector's behavior instead of ak.behavior, and also type hint corrections on allclose—booleans, not floats—that I hadn't noticed before.)

I'll merge it.

@jpivarski jpivarski merged commit ddd2e0a into scikit-hep:main Jun 16, 2023
@Saransh-cpp Saransh-cpp added this to the v1.1.0 milestone Jun 16, 2023
@jpivarski jpivarski mentioned this pull request Jun 22, 2023
7 tasks
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.

Summing over array dimensions does not work properly
4 participants