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

[RFC] streamer type generation #18

Closed
wants to merge 1 commit into from
Closed

Conversation

Moelf
Copy link
Member

@Moelf Moelf commented Jun 30, 2021

I think your original TODO had a typo? we are generating for readfields! it seems.

We come up with a structure so we don't need to type out an entire @eval function .... in the _generate_T_Types. This PR serves as a POC, and at least avoid hard-coding for each n in TType_n variation

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #18 (cd79a1d) into master (870c920) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   77.22%   77.29%   +0.07%     
==========================================
  Files           7        7              
  Lines         878      881       +3     
==========================================
+ Hits          678      681       +3     
  Misses        200      200              
Impacted Files Coverage Δ
src/bootstrap.jl 70.81% <100.00%> (+0.22%) ⬆️
src/streamers.jl 86.05% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870c920...cd79a1d. Read the comment docs.

@tamasgal
Copy link
Member

Yes, there were a few (changing) naming conventions throughout.

Some of the Type_n things will be need though for bootstrapping, i.e. to provide a fixed set of streamer definitions for well known types which are not described inside ROOT files.

I really need to sit down and think about how to restructure and come up with an easy to follow/understand naming convention for read(), readfields() and whatnot ;)

@tamasgal
Copy link
Member

Sorry for spamming; for the sake of completeness I'd like to link our discussion from last year #4

@Moelf
Copy link
Member Author

Moelf commented Jun 30, 2021

my question is there's no way to know about the ROOT built-in types such as TName_1 and _2 without looking at their source code right? at least for quite many basic types

@tamasgal
Copy link
Member

Yes, exactly. That's what we call "bootstrapping". I got most of these hard coded definitions from uproot, that's easier to read compared to the C++ code 😁

@Moelf Moelf marked this pull request as draft June 30, 2021 21:27
@Moelf
Copy link
Member Author

Moelf commented Jun 30, 2021

will we ever encounter TTypes whose name we have never seen before? (i.e. not in ROOT base) We can't generate readfields! in that case because we know nothing about the struct right? (it's self documented by using ROOT base TTypes somehow?)

I actually couldn't find where uproot hard code the basic stuff :(

@tamasgal
Copy link
Member

tamasgal commented Jul 1, 2021

Yes, I think those types are always provided "in source" and not "in file".

The stuff in uproot is spread out. I created a modified version of uproot3 back then, and dumped the classes which were generated during the runtime. Jim made it so that it basically builds a string from all the streamer information from building blocks and the string is then passed to eval(), basically containing the full class definition.
My modified uproot3 version always created a *.py with the name of the class and the version of it. So I could do a uproot.open("whatever.root") and had a folder with all the e.g. TNamed_3.py, TTree_2.py, KM3NET_...py, etc. ;)

@Moelf Moelf closed this Jul 3, 2021
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