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

fix: add highlevel, behavior arguments to composite reducers #2754

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Oct 12, 2023

A number of high-level operations in Awkward Array did not accept the highlevel or behavior arguments. I don't think we ever explicitly decided upon this, so I assume that it is an oversight in need of correction!

@agoose77 agoose77 marked this pull request as draft October 12, 2023 20:49
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #2754 (0b0691e) into main (ea8de12) will increase coverage by 0.06%.
The diff coverage is 97.18%.

Additional details and impacted files
Files Coverage Δ
src/awkward/_layout.py 84.46% <100.00%> (+0.79%) ⬆️
src/awkward/operations/ak_corr.py 100.00% <100.00%> (+12.50%) ⬆️
src/awkward/operations/ak_covar.py 100.00% <100.00%> (+11.11%) ⬆️
src/awkward/operations/ak_linear_fit.py 100.00% <100.00%> (+9.67%) ⬆️
src/awkward/operations/ak_moment.py 88.00% <100.00%> (+0.50%) ⬆️
src/awkward/operations/ak_ptp.py 97.05% <100.00%> (ø)
src/awkward/operations/ak_softmax.py 100.00% <100.00%> (ø)
src/awkward/operations/ak_var.py 82.00% <100.00%> (ø)
src/awkward/operations/ak_mean.py 87.23% <88.88%> (+6.38%) ⬆️
src/awkward/operations/ak_std.py 83.72% <88.88%> (ø)

@agoose77 agoose77 temporarily deployed to docs October 12, 2023 21:02 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs October 12, 2023 21:57 — with GitHub Actions Inactive
@agoose77 agoose77 marked this pull request as ready for review October 13, 2023 09:35
@agoose77 agoose77 temporarily deployed to docs October 13, 2023 09:48 — with GitHub Actions Inactive
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.

I think it was an oversight. The functions you found are all of the "pseudoreducers." They were added as a pack, all at once.

@jpivarski
Copy link
Member

This looks done, it's a straightforward fix, and it has been hours since you touched it, so I'm going to merge it.

@jpivarski jpivarski enabled auto-merge (squash) October 13, 2023 19:10
@jpivarski jpivarski merged commit 2748c0c into main Oct 13, 2023
37 checks passed
@jpivarski jpivarski deleted the agoose77/feat-add-highlevel-args-composite-reducers branch October 13, 2023 19:32
Comment on lines +39 to +63
def maybe_highlevel_to_lowlevel(obj):
"""
Args:
obj: an object

Calls #ak.to_layout and returns the result iff. the object is a high-level
Awkward object, otherwise the object is returned as-is.

This function should be removed once scalars are properly handled by `to_layout`.
"""
import awkward.highlevel

if isinstance(
obj,
(
awkward.highlevel.Array,
awkward.highlevel.Record,
awkward.highlevel.ArrayBuilder,
),
):
return awkward.to_layout(obj)
else:
return obj


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will get rid of this function once our policies for ak.to_layout of scalars are more well defined.

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.

2 participants