-
Notifications
You must be signed in to change notification settings - Fork 76
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
Minor tweak to regex to fix #74. #77
Conversation
Perfect, thanks for the follow-up! |
Ah—they passed because CI isn't testing Dask. That's okay; I'm going to be removing the Dask experiment, anyway. (It puts restrictions on Awkward Arrays that I complained about in scikit-hep/awkward#350.) |
I just tried it when working on another issue, and ROOT does complain:
so it's not 100% allowed. |
OK interesting 😉 Do you plan to change it back or show a warning? At this very moment I don't see any danger in the relaxed regex but maybe encouraging a proper protocol is the better way, meaning that a warning/error is probably more appropriate than silently accepting it 🤔 |
No, ROOT's warning is in the creation of these nameless leaves, but since ROOT doesn't forbid it, there can be files out there that have this feature. We therefore need to be able to read it. This regex was one consequence of that; there might be others. If, for instance, we identify leaves as fields by name, we'd have to introduce substitute names. (I haven't checked to see whether ROOT already does that at creation-time.) |
Yep I see, you're right! |
This is indeed very interesting.
So ROOT only throws a warning when using nameless leaves for basic variables (i.e. non-arrays) like in the Branch
Concerning the naming of the leaves this seems to behave in a very interesting behavior. While in |
@tamasgal and @Bernd1995, this fixes #74 and therefore provides a good model for JuliaHEP/UnROOT.jl#9. The information about a branch's dimensions are stored in the TLeaf's fTitle, and that needs to be parsed by a regex. The example @Bernd1995 created had no leaf names:
as opposed to
(where the leaf name is
"el"
), which I guess is legal, so the regex has to allow for that.This CI is going to fail until I fix scikit-hep/awkward#416, which @tamasgal pointed out in another issue.