Skip to content

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Mar 5, 2021

Opening this to make it easier to compare but this is incredibly painful :D See #38 for lengthier details for now.

>>> [i.members for i in eff.member('fBeta_bin_params').tolist()]
[{'first': -1.0, 'second': -2.0}, {'first': 2.0, 'second': 4.0}, {'first': 4.0, 'second': 8.0}, {'first': 8.0, 'second': 16.0}, {'first': 16.0, 'second': 32.0}, {'first': 32.0, 'second': 64.0}, {'first': 64.0, 'second': 128.0}, {'first': 128.0, 'second': 256.0}, {'first': 256.0, 'second': 512.0}, {'first': 512.0, 'second': 1024.0}, {'first': 1024.0, 'second': 2048.0}, {'first': -1.0, 'second': -2.0}, {'first': 1.0, 'second': 1.0}]

@kratsg
Copy link
Contributor Author

kratsg commented Mar 5, 2021

make the ROOT file

$ cat make_tefficiency.py 
import ROOT

fp = ROOT.TFile.Open("test-efficiency.root", "RECREATE")

nbins = 11

h_den = ROOT.TH1F('h_den', 'h_den', nbins, 0, 100)
h_num = ROOT.TH1F('h_num', 'h_num', nbins, 0, 100)

for i in range(1, nbins):
    h_num.SetBinContent(i, 2**i)
    h_den.SetBinContent(i, 2**(i+1))

eff = ROOT.TEfficiency(h_num, h_den)
eff.SetName('TEfficiencyName')
eff.SetTitle('TEfficiencyTitle')

eff.SetBetaBinParameters(0, -1.0, -2.0)
for i in range(1, nbins):
    eff.SetBetaBinParameters(i, 2**i, 2**(i+1))

eff.SetBetaBinParameters(nbins, -1.0, -2.0)

h_den.Write()
h_num.Write()
eff.Write()
fp.Close()

then run it

$ cat debug.py 
import uproot

with uproot.open('test-efficiency.root') as fp:
    eff = fp['TEfficiencyName']

Comment on lines 807 to 809
# there's extra stuff, maybe?
_num_memberwise_bytes = cursor.field(chunk, _stl_container_size, context)
_something_else = cursor.field(chunk, struct.Struct(">H"), context)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This portion that we're at right now looks like

--+---+---+---+---+---+---+---+---+---+
  0   0   0 215 190 210   0   0   0  13  
--- --- --- --- --- --- --- --- --- --- 

Unclear what the first 6 bytes are here, but the last 4 seem to be number of elements in the vector (length) in the line below. I assumed it might be 4 bytes / 2 bytes but still not sure what they're meant to match up with.

Comment on lines 816 to 821
# make a shell
values = numpy.empty(length, dtype=_stl_object_type)
for i in uproot._util.range(length):
values[i] = model.read(
chunk, cursor, {**context, "reading": False}, file, selffile, parent
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we make a shell, which means we skip actually reading anything out (in this case, this means "moving the cursor"). We'd just want to inflate all of our classes/objects so we can ask them to each read the file one by one to fill member-wise (see next block).

@kratsg kratsg marked this pull request as ready for review March 8, 2021 19:26
@kratsg kratsg requested a review from jpivarski March 8, 2021 19:32
@kratsg kratsg added the feature New feature or request label Mar 8, 2021
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.

Great! I found a few things to clean up, but I'll be committing them directly to this PR and then merging.

Thanks!

uproot/model.py Outdated
Comment on lines 764 to 766
self.hook_before_read(
chunk=chunk, cursor=cursor, context=context, file=file
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to take this hook out of the if context.get("reading", True) because we're already in the read method. The read_members method, on the other hand, gets entirely skipped.

…fail tests because syntatically correct Python was generated, but it wouldn't have worked if we ever needed to call 'read_member_n' on those classes.)
@jpivarski jpivarski merged commit deaaf9f into main Mar 9, 2021
@jpivarski jpivarski deleted the feat/memberwise branch March 9, 2021 18:32
@kratsg kratsg mentioned this pull request Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants