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: check validity error properly #2550

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

agoose77
Copy link
Collaborator

The parameters check for validity error does not recurse in the existing implementation

@agoose77 agoose77 requested a review from jpivarski June 29, 2023 14:31
@agoose77 agoose77 temporarily deployed to docs-preview June 29, 2023 14:42 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #2550 (4cba0f5) into main (1f6041e) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_do.py 83.64% <ø> (+0.31%) ⬆️
src/awkward/contents/content.py 76.00% <ø> (-0.04%) ⬇️
src/awkward/contents/indexedoptionarray.py 88.32% <33.33%> (-0.28%) ⬇️
src/awkward/contents/indexedarray.py 78.99% <100.00%> (+0.14%) ⬆️

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 don't think it was failing to recurse before: my reading of the red/removed code is that it would only stop recursion if it hit an error, which is what it's supposed to do. (I didn't construct a test, though.)

Anyway, the new code also looks correct but it's better from an OOP point of view.

This is a simple PR and it looks like you're done; I'll merge it immediately.

@jpivarski jpivarski merged commit 2cc0604 into main Jun 29, 2023
@jpivarski jpivarski deleted the agoose77/fix-validity-error-categorical branch June 29, 2023 21:34
@agoose77
Copy link
Collaborator Author

I don't think it was failing to recurse before: my reading of the red/removed code is that it would only stop recursion if it hit an error, which is what it's supposed to do. (I didn't construct a test, though.)

The intention was that we have cross-layout checks in a separate method. However, only ak._do.validity_error checked this for the top-level layout (hence the test). We might still want a dedicated ak.Content._validity_error_parameters function in future (if we have such checks), but this is L3 and we don't currently need it.

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