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: cartesian nested record #2929

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jan 10, 2024

Fixes #2928, by normalising string field names to integers.

This code could be further refactored; it's becoming clear that our recursively_apply abstraction would benefit from better support for smarter continuations (i.e. re-entrant with a different callable). That can be tackled later, though.

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've been scanning over the code and I don't see how it changes things. Previously, the name nested was reused, which is bad practice—naming the possibly-new list nested_as_index is a good idea. But I didn't see any further uses of nested that were previously getting clobbered.

Is it a function-scoping issue? Python has a gotcha in which functions defined in a loop that close over a variable that changes in the loop don't end up with the value of the variable at the time when the function was defined, but at the time when the function is called—that can be tricky. But there aren't any loops here.

I don't see how it does it, but it evidently does make a difference. Some of the tests are failing: are they stale expected values, derived based on the old, wrong behavior?

@agoose77
Copy link
Collaborator Author

@jpivarski I introduced a new bug with this PR — I changed tack during fixing of the bug and must have stepped too far back in the editor history. I had thought that such a simple fix would not break anything else, so I didn't touch the local testing and let the CI handle it! That shows me!

As for the change itself, the issue was indeed that we're using nested to mean too many things. Now, it's clear that we test indices against nested_as_index, which ensures the cause of the original bug (test for axis name in list of integers) can't happen.

@agoose77 agoose77 enabled auto-merge (squash) January 11, 2024 07:29
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e86b149) 81.94% compared to head (1cb67ec) 81.95%.

Additional details and impacted files
Files Coverage Δ
src/awkward/operations/ak_cartesian.py 91.00% <100.00%> (+0.37%) ⬆️

@agoose77 agoose77 merged commit 5d16503 into main Jan 11, 2024
38 checks passed
@agoose77 agoose77 deleted the agoose77/fix-cartesian-nested-record branch January 11, 2024 08:48
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.

ak.cartesian(..., nested=True) results in behaviour expected from nested=False
2 participants