Issues with (Char_t) seediEtaOriX in Nanov11 onwards

[a-kapoor]

The PR #40327 added iEta,iPhi (for barrel) and iX,iY(for endcaps) to NanoAOD for photons and electrons. These are useful to veto the EE+ leak region at analysis level.

These variables were added as int8, it is not clear why _seediEtaOriX ends of as Char_t in the NanoAOD.
See docs

Same issue for electrons too.

The consequence is that when one reads these values, the _seediEtaOriX dumps some junk values:

Happens for both photons and electrons. The reason might be that ROOT interprets int8 as char. Although it is not clear why it only happens for _seediEtaOriX and not _seediPhiOriY

A workaround for analyzers might be to force ROOT to tree it as an integer, like this:

Note how I forced the type conversion by adding a zero.

Yet a permanent solution for future versions could be to change the type of this variable from int8 to int16.

This is not an isolated issue, and my student has seen other issues in new NanoAODs with these kind of datatypes:
https://cms-talk.web.cern.ch/t/uchar-short-and-char-variables-in-nanoaod-v12-inconsistent-results-using-root/33208

Will target a fix for EGM variable in Nano v14.

@RSalvatico @swagata87 @Prasant1993

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @a-kapoor Anshul Kapoor.

@Dr15Jones, @makortel, @smuzaffar, @antoniovilela, @rappoccio, @sextonkennedy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[a-kapoor]

type egamma, root

[a-kapoor]

assign egamma-pog, xpog

[cmsbuild]

New categories assigned: egamma-pog,xpog

@vlimant,@hqucms,@a-kapoor,@RSalvatico you have been requested to review this Pull request/Issue and eventually sign? Thanks

[makortel]

The definitions (as added by #40327)

cmssw/PhysicsTools/NanoAOD/python/electrons_cff.py

Lines 353 to 354 in f5cbbf1

	seediEtaOriX = Var("superCluster().seedCrysIEtaOrIx","int8",doc="iEta or iX of seed crystal. iEta is barrel-only, iX is endcap-only. iEta runs from -85 to +85, with no crystal at iEta=0. iX runs from 1 to 100."),
	seediPhiOriY = Var("superCluster().seedCrysIPhiOrIy",int,doc="iPhi or iY of seed crystal. iPhi is barrel-only, iY is endcap-only. iPhi runs from 1 to 360. iY runs from 1 to 100."),

have

• seediEtaOriX with type "int8"
• seediPhiOriY with type int

that seems explain why the first one is stored as Char_t and the second as Int_t.

[vlimant]

indeed, the new types were introduced with #40478 and this end up with leaves of questionable type in ROOT
root [27] Events->GetLeaf("Photon_seediEtaOriX")->GetTypeName() (const char *) "Char_t" root [28] Events->GetLeaf("Photon_seediPhiOriY")->GetTypeName() (const char *) "Int_t"

so int8 (and uint8) are messed up when transferred to plain ROOT, as was already noticed by @swertz (not CMS anymore).

[makortel]

Otherwise the behavior looks like expected. int8_t is (likely) a type alias to signed char, and out of the box that gets interpreted as a character (rather than integer) when printing out.

[makortel]

type -root

I don't think this is an issue to be addressed by ROOT.

[vlimant]

@makortel : where do I find the real meaning of "int8_t" and can we change this to what it means ?

[makortel]

where do I find the real meaning of "int8_t" and can we change this to what it means ?

The int8_t is defined in the standard library, in our case libstdc++, which seems to take them directly from system glibc stdint.h, that on EL8 defines it as

// in /usr/include/bits/types.h
typedef signed char __int8_t;

// in /usr/include/bits/stdint-intn.h
typedef __int8_t int8_t;

We can not change its meaning. (What would you change it to? signed char is the fundamental "at least 8-bit" type, that on 64-bit Linux is exactly 8 bits)

[a-kapoor]

@vlimant Shall I change the "int8" to int as done for seediPhiOriY if there are no central nano issues with this? It will solve the problem for these variables at least. What do you think?

[swertz]

Hi there, if you want to avoid that pesky ROOT formatting issue with int8, just change it to int16 (see indeed description in #40478).

uint8 should be fine but that is not what you want here.

[vlimant]

because of the fundamental type and how it ends up stored in root, I think we need to centrally roll back and ban usage of int8, unit8, and maybe others from the nano table code to avoid dubious interpretation in root.
unless there is a smarter way to tell root to interpret signed char as integer number automatically

[swertz]

See the discussion here: https://root-forum.cern.ch/t/draw-short-integers-as-numbers-not-characters/52530/24

int8 should be fine, it's really only uint8 that's making a mess.

[a-kapoor]

Hi @swertz as you can see in my description, in this specific case, it is "int8" that's making the mess

cmssw/PhysicsTools/NanoAOD/python/electrons_cff.py

Lines 353 to 354 in f5cbbf1

	seediEtaOriX = Var("superCluster().seedCrysIEtaOrIx","int8",doc="iEta or iX of seed crystal. iEta is barrel-only, iX is endcap-only. iEta runs from -85 to +85, with no crystal at iEta=0. iX runs from 1 to 100."),
	seediPhiOriY = Var("superCluster().seedCrysIPhiOrIy",int,doc="iPhi or iY of seed crystal. iPhi is barrel-only, iY is endcap-only. iPhi runs from 1 to 360. iY runs from 1 to 100."),

[swertz]

Sorry that was a typo, I meant to say that uint8 should be fine but int8 should be avoided and int16 used instead.

[vlimant]

uint8 (used for PV_npvs for example) give issues with pyroot https://cms-talk.web.cern.ch/t/issues-with-uint8-t-variables-in-nanoaodv12/31598

[vlimant]

parallel to rolling back int8 in nano (and then uint8), how do we go about having this "fixed" at the ROOT level ?

[vlimant]

root-project/root#14361 for reference

[vlimant]

+1

[vlimant]

please close