RNO-G / mattak

RNO-G dataformats
1 stars 1 forks source link

add residual variance as a TGraph complementary to residual TGraphs #36

Closed CamphynR closed 3 months ago

CamphynR commented 4 months ago

Addition of a TGraph containing the residual variance per DAC to complement the residual means per DAC

cozzyd commented 4 months ago

This looks fine to me as is, but have you considered using a TGraphErrors instead (i.e. have the residual variance stored as the "y-error" for each value?). Not sure if that's intuitive to anybody but me. It's maybe slightly better for plotting (since you essentially get error bars when plotting) but maybe that's a niche use case.

Note that this introduces a new memory leak (alongside existing memory leaks...). I think I started fixing those on a private branch of mine, once this is merged I can fix it here too.

martin01021207 commented 4 months ago

I think it's a very good idea to use TGraphErrors to show error bars as Cosmin's suggested, so maybe we should change the original TGraph graph_residAve to TGraphErrors, then we should also calculate the standard deviation from the variance, and then add them to the y errors in graph_residAve. But keep the graph_residVar as it is?

CamphynR commented 4 months ago

I changed it to be TGraphErrors which stores the std in the yerrors , I did notice that it is quite difficult to read a TGraphErrors object with uproot (apparently that functionality has not been implemented yet, but I suspect there should be a way). I don't know how important it is to have readability with uproot, but if it's too difficult to simply read in the values we could include the TGraphs as well (although this seems like redundant storing)?

martin01021207 commented 4 months ago

It looks great to me, but unfortunately I do not know the answer regarding to the use of TGraphErrors with uproot :(

fschlueter commented 4 months ago

@CamphynR have you tried reading the TGraphError objects as TGraphAsymmErrors? It seems that this might be implemented in uproot.

cozzyd commented 4 months ago

uproot doesn't have a specialization (decoration? I forgot what word uproot uses) for TGraphErrors, but you can still access all of it's members as such (typos elided..) :

In [1]: import uproot
In [2]: f = uproot.open('test.root')
In [4]: g = f['ge']
In [5]: g.all_members
Out[5]: 
{'@fUniqueID': 0,
 '@fBits': 16778248,
 'fName': 'ge',
 'fTitle': 'Test TGraphErrors',
 'fLineColor': 1,
 'fLineStyle': 1,
 'fLineWidth': 1,
 'fFillColor': 0,
 'fFillStyle': 1000,
 'fMarkerColor': 1,
 'fMarkerStyle': 1,
 'fMarkerSize': 1.0,
 'fNpoints': 10,
 'fX': array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.]),
 'fY': array([ 0.,  1.,  4.,  9., 16., 25., 36., 49., 64., 81.]),
 'fFunctions': <TList of 0 items at 0x7f3e4b6edd90>,
 'fHistogram': <TH1F (version 3) at 0x7f3e492edaf0>,
 'fMinimum': -1111.0,
 'fMaximum': -1111.0,
 'fEX': array([0., 0., 0., 0., 0., 0., 0., 0., 0., 0.]),
 'fEY': array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.])}

In [7]: g.all_members['fX']
Out[7]: array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.])

In [8]: g.all_members['fY']
Out[8]: array([ 0.,  1.,  4.,  9., 16., 25., 36., 49., 64., 81.])

In [9]: g.all_members['fY'].dtype
Out[9]: dtype('>f8')

In [10]: g.all_members['fEY']
Out[10]: array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.])

In [11]: g.all_members['fEX']
Out[11]: array([0., 0., 0., 0., 0., 0., 0., 0., 0., 0.])
cozzyd commented 4 months ago

(there is also a members dict, but that doesn't include things inherited from TGraph as far as I can tell).

CamphynR commented 4 months ago

Ow that's perfect thanks! Then the use of TGraphErrors in uproot is quite intuitive if you know the correct syntax :)

cozzyd commented 4 months ago

I went ahead and implemented these changes. Maybe someone can check I didn't do something super dumb, but otherwise I want to merge soon so that I can then incorporate some other changes in the memory-fixes branch

martin01021207 commented 3 months ago

I went ahead and implemented these changes. Maybe someone can check I didn't do something super dumb, but otherwise I want to merge soon so that I can then incorporate some other changes in the memory-fixes branch

@cozzyd I suppose we should go ahead and merge? Unless @CamphynR or @fschlueter have some other suggestions?

cozzyd commented 3 months ago

Just merged!