cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.28k forks source link

Candidate ParticleState data member access in fwlite shows incorrect values (Double32_t) #29373

Open slava77 opened 4 years ago

slava77 commented 4 years ago

originally reported by @mmasciov

when plotting/scanning values from GenMET we see different results when displaying values from bare root (without FWLite) compared to root setup coming with CMSSW.

I have an old example for CMSSW_10_6_0 for a file available on cmsdev34:/build/slava77/step3_inAODSIM.root (the symptoms are present in at least as late as 11_1_0_pre2 using more recent files as well)

  1. bare root source /cvmfs/cms.cern.ch/slc7_amd64_gcc700/lcg/root/6.14.09-pafccj/etc/profile.d/init.sh then in root session:

    Events->Scan("recoGenMETs_genMetTrue__HLT.obj[0].m_state.p4Polar_.fCoordinates.fPt")
    ************************
    *        0 * 3.676e-11 *
    *        1 * 124.63149 *
    *        2 * 7.5937628 *
  2. from CMSSW_1_6_0

    Events->Scan("recoGenMETs_genMetTrue__HLT.obj[0].m_state.p4Polar_.fCoordinates.fPt")
    ************************
    *        0 * 2.5198919 *
    *        1 * 1.9971023 *
    *        2 *  1.111889 *

    compare this to the normal function call returning expected values, in agreement with the bare root case

    Events->Scan("recoGenMETs_genMetTrue__HLT.obj[0].pt()")
    ************************
    *        0 * 3.676e-11 *
    *        1 * 124.63149 *
    *        2 * 7.5937628 *

Note that the branch is not fully split. The branch of interest comes with the following specs in the dictionaries:

   <field name="p4Polar_" iotype="ROOT::Math::LorentzVector<ROOT::Math::PtEtaPhiM4D<Double32_t> >" />

while the C++ representation is ROOT::Math::LorentzVector<ROOT::Math::PtEtaPhiM4D<double> >

It looks like what's happening is in the bare root case the type conversion is done correctly, while with FWLite we are getting Double32_t read or display incorrectly.

IIUC, with an explicit function call recoGenMETs_genMetTrue__HLT.obj[0].pt() the full object is streamed in memory and at this level we get a correct layout of data and a correct result.

@pcanal

cmsbuild commented 4 years ago

A new Issue was created by @slava77 Slava Krutelyov.

@Dr15Jones, @silviodonato, @dpiparo, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

slava77 commented 4 years ago

assign reconstruction,core

cmsbuild commented 4 years ago

New categories assigned: core,reconstruction

@Dr15Jones,@smuzaffar,@slava77,@perrotta,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

pcanal commented 4 years ago

@slava77devel Can you remind me how to setup for the failing example with a debug version of ROOT and CMSSW (since apparently something there is interfering)? Thanks.

slava77 commented 4 years ago

@slava77devel Can you remind me how to setup for the failing example with a debug version of ROOT and CMSSW (since apparently something there is interfering)? Thanks.

I'd have to redirect to @Dr15Jones @makortel @smuzaffar on how to get a CMSSW setup with a debug version of ROOT

slava77 commented 4 years ago

@slava77devel Can you remind me how to setup for the failing example with a debug version of ROOT and CMSSW (since apparently something there is interfering)? Thanks.

I'd have to redirect to @Dr15Jones @makortel @smuzaffar on how to get a CMSSW setup with a debug version of ROOT

Perhaps this can be discussed in the core meeting today?

slava77 commented 4 years ago

@pcanal I found out that our rootmaster builds include root debug symbols. I used CMSSW_11_1_ROOT6_X_2020-04-06-2300 and I was able to reproduce the same problem as in point 2 of this issue description https://github.com/cms-sw/cmssw/issues/29373#issue-592151531

e.g.

en->Scan("recoGenMETs_genMetTrue__HLT.obj[0].m_state.p4Polar_.fCoordinates.fPt:recoGenMETs_genMetTrue__HLT.obj[0].pt()")
************************************
*    Row   * recoGenME * recoGenME *
************************************
*        0 * 2.5198919 * 3.676e-11 *
*        1 * 1.9971023 * 124.63149 *
*        2 *  1.111889 * 7.5937628 *
*        3 * 0.6871746 * 0.8779900 *
*        4 * -1.770822 * 17.392650 *
pcanal commented 4 years ago

Cool. Still can you remind me the steps :)

slava77 commented 4 years ago

Cool. Still can you remind me the steps :)

since the file is on cmsdev34, go there

cmsrel CMSSW_11_1_ROOT6_X_2020-04-06-2300
cd CMSSW_11_1_ROOT6_X_2020-04-06-2300
cmsenv
root

and then in the root session:

en = new TChain("Events");
en->Add("/build/slava77/step3_inAODSIM.root");
en->Scan("recoGenMETs_genMetTrue__HLT.obj[0].m_state.p4Polar_.fCoordinates.fPt:recoGenMETs_genMetTrue__HLT.obj[0].pt()")

The two columns should show the same values because at c++ level (after following inlined intermediate methods) reco::GenMET::pt() { return m_state.p4Polar_.fCoordinates.fPt;}

[but the values are different, suggesting of a problem in loading the data member directly]

pcanal commented 4 years ago

I can reproduce it. Checking into it but on first impression it looks like it'll take a while.

slava77 commented 4 years ago

@pcanal please let me know if there were updates on this issue. Thank you.

slava77 commented 4 years ago

@pcanal please let me know if there were updates on this issue. Thank you.

slava77 commented 4 years ago

@pcanal please let me know if there were updates on this issue. Thank you.