Skip to content

Change axis default on a number of functions in uproot-raw codegen #1054

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

ponyisi
Copy link
Collaborator

@ponyisi ponyisi commented May 10, 2025

We make many awkward functions available when writing cuts and expressions for uproot. A lot of these default to operating on the entire array at once (axis=0) in ways that make no sense for an event-by-event cut mask. In general this default causes a lot of errors. We will override those defaults here (we believe the defaults are so incorrect that nobody with correct code will be affected by this). The axis can still be overridden if people want. The list of functions with changed defaults will be given in the frontend documentation.

Copy link
Collaborator

@gordonwatts gordonwatts left a comment

Choose a reason for hiding this comment

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

I really don't like this. I get why you are doing this. But this is magic and now behaves differently than regular code. So what users learn will break here.

btw - if axis argument is specified twice will this work?

@ponyisi
Copy link
Collaborator Author

ponyisi commented May 13, 2025

It will work if the user specifies axis again. (It doesn't work if a user specifies it as a positional argument instead of a keyword one but this is a very rare pattern.)

I understand why this would make one uncomfortable but the fact is that essentially every new user I've seen work with the uproot-raw transformer makes the error of not specifying axis=1 in a relevant place and unless they're quite competent with awkward they don't figure it out on their own. When awkward specifies axis=0 as a default this is never the behavior you want here (unless you can come up with a circumstance where we want queries to correlate across events ...). The biggest likelihood of causing problems, I would guess, is actually that some inexperienced users come up with some cut expression that works in uproot-raw and not when they run awkward locally.

The equivalent RDataFrame expressions don't need this kind of silliness to work correctly...

@ponyisi
Copy link
Collaborator Author

ponyisi commented May 13, 2025

I suppose we could make it an option in the uproot-raw request (though I would still want the default to be the override).

@gordonwatts
Copy link
Collaborator

I suppose we could make it an option in the uproot-raw request (though I would still want the default to be the override).

Ok - that makes sense to me. In the documentation could we also make it very clear?

I guess the fundamental problem is that awkard was designed for multi-dimensional arrays. RDF was defined for HEP events of multi-dimensional arrays. func-adl does this too, implicitly.

@BenGalewsky
Copy link
Contributor

@ponyisi please update the docs as part of this PR

@ponyisi
Copy link
Collaborator Author

ponyisi commented Jun 19, 2025

@BenGalewsky the docs will go into the servicex_frontend anyway, no?

@BenGalewsky
Copy link
Contributor

@BenGalewsky the docs will go into the servicex_frontend anyway, no?

Ah, right

@ponyisi
Copy link
Collaborator Author

ponyisi commented Jun 20, 2025

@BenGalewsky Documentation in ssl-hep/ServiceX_frontend#606 . I also added a query option to allow the user to revert this change, as discussed in the Thursday meeting. Both modes have been tested on testing-3.

Copy link
Contributor

@kyungeonchoi kyungeonchoi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@kyungeonchoi
Copy link
Contributor

@BenGalewsky Documentation in ssl-hep/ServiceX_frontend#606 . I also added a query option to allow the user to revert this change, as discussed in the Thursday meeting. Both modes have been tested on testing-3.

It would be also nice to specify the ServiceX backend version with this change in the client documentation ssl-hep/ServiceX_frontend#606

@ponyisi ponyisi merged commit f147f20 into develop Jun 25, 2025
75 checks passed
@ponyisi ponyisi deleted the uproot-raw-axis-1 branch June 25, 2025 17:58
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.

4 participants