Skip to content

fix: ak.argcombinations should allow negative axis#3301

Merged
jpivarski merged 3 commits into
mainfrom
jpivarski/allow-negative-axis-in-argcombinations
Nov 13, 2024
Merged

fix: ak.argcombinations should allow negative axis#3301
jpivarski merged 3 commits into
mainfrom
jpivarski/allow-negative-axis-in-argcombinations

Conversation

@jpivarski

Copy link
Copy Markdown
Member

No description provided.

@pfackeldey

Copy link
Copy Markdown
Collaborator

@jpivarski looks good to me!
I wonder: do you know why this was forbidden in the first place?

@jpivarski

Copy link
Copy Markdown
Member Author

I think it's just always been an oversight.

  • ak.argcombinations and ak.combinations were ported over from the Awkward 1 C++ in C++ refactoring: ak.argcombinations, ak.combinations #1307, and ak.argcombinations has the axis >= 0 restriction there, while ak.combinations doesn't. ak.argcombinations was written in terms of ak.combinations and ak.localindex, as it is now.
  • ak.argchoose and ak.choose (the old names) were orinally written in Awkward 1 in argchoose and choose. #160, and ak.argchoose has the axis >= 0 restriction there, while ak.choose doesn't. ak.argchoose was written in terms of ak.combinations and localindex, as it is now.
  • argchoose and choose didn't have an axis argument in Awkward 0. (choose was written in terms of argchoose, by applying the argchoose result as an integer slice, so that's different.)

When I was adding axis arguments to everything in the Awkward 0 → 1 transition, I must have thought, "There's no way this is well-defined (or implementable) if it has an axis argument" for argchoose but then had a different thought in choose. I probably never looked at them side by side like this. It is not the case that they originally both had the restriction and it was lifted from choose. So, it's not a maintenance inconsistency; it's a development inconsistency.

@jpivarski
jpivarski enabled auto-merge (squash) November 13, 2024 15:41
@jpivarski
jpivarski merged commit ab484af into main Nov 13, 2024
@jpivarski
jpivarski deleted the jpivarski/allow-negative-axis-in-argcombinations branch November 13, 2024 16:08
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.

Argcombinations not taking negative axis values

2 participants