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: not all branchid_interpretation are necessarily in ranges_or_baskets #1202

Merged
merged 2 commits into from
May 5, 2024

Conversation

jpivarski
Copy link
Member

This is an addendum to PR #838, which fixed #833 by checking for CannotBeAwkward errors earlier (before reading data).

The previous PR added a TBranch._awkward_check on all TBranches in branchid_interpretation, but since there wasn't a list of actual TBranch objects (with methods like _awkward_check that you can call), the previous PR made a lookup dict called branchid_to_branch for this purpose, getting the TBranch.cache_key ("branch id") to TBranch object mapping from ranges_or_baskets.

However, it looks like ranges_or_baskets can have fewer branches than branchid_interpretation. Obviously I didn't think so when I wrote PR #8381, but the bug @gordonwatts found can only have happened if this is the case. It doesn't seem implausible: the branchid_interpretation list is made by iterating over the user-provided expressions while ranges_or_baskets includes only those branches that are really going to get downloaded, decompressed, and interpreted. It is plausible that a step in between filters them, though I can only see how filtering could go in this direction: more branchid_interpretation branches than ranges_or_baskets branches.

Given that, and the fact that the purpose of this code is just to raise an error earlier than it would get raised anyway, and the branchid_to_branch mapping is not used for anything else, I think it is safe and correct to simply patch this by skipping the branches in branchid_interpretation that aren't in ranges_or_baskets. That's all this PR does.

Footnotes

  1. Since fix: complain about CannotBeAwkward earlier, before reading data. #838 was itself a patch, made long after the original writing of the code, I can't say that I knew better then and had some good reason for thinking that branchid_interpretation and ranges_or_baskets should have exactly the same set of branches.

@jpivarski jpivarski enabled auto-merge (squash) May 5, 2024 15:23
@jpivarski jpivarski merged commit f067005 into main May 5, 2024
24 checks passed
@jpivarski jpivarski deleted the jpivarski/fix-the-bug-gordon-found-on-slack branch May 5, 2024 15:34
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.

Error in untested AwkwardForth case
1 participant