[muon] a workaround for enum to int conversion problem in pyROOT #26446

drkovalskyi · 2019-04-12T14:07:36Z

There is a subtle problem with python type conversion from signed enums to basic types. In some cases it converts negative enums to an unsigned int. For example instead of -10 you get 4294967286. This problem was report to ROOT team, but it was never fixed.
It's very easy to miss this problem and unfortunately one cannot rely on enum checks to avoid it, since for example ROOT.reco.GhostMuonFromGaugeOrHiggsBoson evaluates to -10 as it should, but simExtType() returns 4294967286.
The current workaround is documented on Muon POG twiki, but it's not a reliable solution. People (including me keep rediscovering this problem).
https://twiki.cern.ch/twiki/bin/view/CMS/SWGuideMuonIdRun2#SimHit_Muon_matching_since_CMSSW

The pull request provides a trivial fix - return int instead of the enum. I know it's ugly, but getting wrong physics results is worse.

cmsbuild · 2019-04-12T14:08:11Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-04-12T14:14:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26446/9250

This PR adds an extra 20KB to repository

cmsbuild · 2019-04-12T14:15:22Z

A new Pull Request was created by @drkovalskyi for master.

It involves the following packages:

DataFormats/PatCandidates

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@gpetruc, @cbernet, @gouskos, @rovere, @hatakeyamak this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

slava77 · 2019-04-12T14:29:22Z

@cmsbuild please test

cmsbuild · 2019-04-12T14:29:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34179/console Started: 2019/04/12 16:30

drkovalskyi · 2019-04-12T15:15:13Z

FYI @bmahakud, @folguera

slava77 · 2019-04-12T15:17:54Z

@folguera
is this the fix in sim matching that you mentioned yesterday or is this something else?

drkovalskyi · 2019-04-12T15:33:16Z

No, the sim-hit matching problem is still not understood. It's a different bug/problem/feature.

cmsbuild · 2019-04-12T17:39:35Z

+1
Tested at: 415bea7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26446/34179/summary.html

cmsbuild · 2019-04-12T17:39:40Z

Comparison job queued.

cmsbuild · 2019-04-12T19:04:03Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26446/34179/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3140495
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3140297
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

slava77 · 2019-04-13T10:24:12Z

DataFormats/PatCandidates/interface/Muon.h

+      /// returns reco::MuonSimType - we cast it to int to avoid issues in pyROOT
+      int simType() const { return simType_; }
+      /// reco::ExtendedMuonSimType - we cast it to int to avoid issues in pyROOT
+      int simExtType() const { return simExtType_; }


This solution degrades type safety to serve a broken external use case.

I propose to introduce/add simTypeInt and simExtTypeInt with appropriate comments that it's to be used in FWLite environment where the scoped enums are not properly recognized.

I don't think your solution will work well because the user needs to know that he has to use simTypeInt to be safe. I have personally rediscovered this problem 3 times, i.e. one tends to forget about it and use it as is. All these accessors are primarily used by endusers, i.e. there won't be many use cases where someone would really benefit from type safety. In other words the tradeoff is between a benefit that won't be used vs a real problem that affects a number of users.

slava77 · 2019-04-13T12:20:24Z

This problem was report to ROOT team, but it was never fixed.

please share the ROOT JIRA issue number for this.

cmsbuild · 2019-08-05T09:31:01Z

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmsdist#5130
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1829/console Started: 2019/08/05 11:32

cmsbuild · 2019-08-06T04:21:26Z

+1
Tested at: 415bea7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dd81cd/1829/summary.html

cmsbuild · 2019-08-06T04:21:33Z

Comparison job queued.

cmsbuild · 2019-08-06T06:47:29Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dd81cd/1829/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-dd81cd/11634.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 2715989
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2715659
DQMHistoTests: Total skipped: 329
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

slava77 · 2019-09-03T17:51:47Z

Here is a simple example: /afs/cern.ch/user/d/dmytro/public/forSlava/test_pyroot.py
When it works right you should get simType to be -10, when it's not you get 4294967294.
It reads data available at CERN, so it's better to run from CERN.

I tested a slightly modified version in CMSSW_11_0_ROOT6_X_2019-09-03-0600

#!/bin/env python
import os, re, sys, time, ROOT, subprocess
from DataFormats.FWLite import Events, Handle

muonHandle, muonLabel = Handle("std::vector<pat::Muon>"),"slimmedMuons"

ROOT.gROOT.SetBatch(True)
events = Events("root://cms-xrd-global.cern.ch//store/group/phys_muon/dmytro/tmp/store+mc+RunIIAutumn18MiniAOD+DYJetsToLL_M-50_TuneCP5_13TeV-madgraphMLM-pythia8+MINIAODSIM+102X_upgrade2018_realistic_v15-v1+80000+695EE995-7CA9-5A46-81EF-C6777A74C791.root")
nevents = 0

print "ROOT.reco.MatchedMuonFromGaugeOrHiggsBoson  = ", ROOT.reco.MatchedMuonFromGaugeOrHiggsBoson
print "ROOT.reco.GhostMuonFromGaugeOrHiggsBoson  = ", ROOT.reco.GhostMuonFromGaugeOrHiggsBoson
print "ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson  = ", ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson

for event in events:
    if nevents>=100: break
    nevents+=1
    event.getByLabel(muonLabel, muonHandle)
    muons = muonHandle.product()
    for muon in muons:
        if muon.simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson or muon.simExtType()>1e4 or muon.simExtType()<0:
            print "simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson or pos/neg: pt: %0.1f \tsimType: %d \textSimType: %d" % (muon.pt(),muon.simType(),muon.simExtType())
        if muon.simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson:
            print "simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson: pt: %0.1f \tsimType: %d \textSimType: %d" % (muon.pt(),muon.simType(),muon.simExtType())
        if muon.simExtType()==ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson:
            print "simExtType()==ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson: pt: %0.1f \tsimType: %d \textSimType: %d" % (muon.pt(),muon.simType(),muon.simExtType())

The output now seems to make sense:

ROOT.reco.MatchedMuonFromGaugeOrHiggsBoson  =  10
ROOT.reco.GhostMuonFromGaugeOrHiggsBoson  =  -10
ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson  =  -10
simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson or pos/neg: pt: 1.1      simType: -4     extSimType: -10
simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson: pt: 1.1         simType: -4     extSimType: -10
simExtType()==ROOT.reco.ExtendedMuonSimType.GhostMuonFromGaugeOrHiggsBoson: pt: 1.1     simType: -4     extSimType: -10
simExtType()==ROOT.reco.GhostMuonFromGaugeOrHiggsBoson or pos/neg: pt: 1.0      simType: -2     extSimType: -4

this also apparently works in root 6.18 branch e.g. in CMSSW_11_0_ROOT618_X_2019-09-03-0600

@drkovalskyi
please double-check that the issue is solved (either in CMSSW_11_0_ROOT618_X_2019-09-03-0600 or in CMSSW_11_0_ROOT6_X_2019-09-03-0600).
Thank you.

slava77 · 2019-09-06T12:30:42Z

#26446 (comment)

@drkovalskyi
please double-check that the issue is solved (either in CMSSW_11_0_ROOT618_X_2019-09-03-0600 or in CMSSW_11_0_ROOT6_X_2019-09-03-0600).
Thank you.

@drkovalskyi
please let me know if you had a chance to check.
Thank you.

drkovalskyi · 2019-09-09T11:48:40Z

It all looks good in my tests.

slava77 · 2019-11-01T13:49:37Z

-1

this is no longer necessary after the root update available starting from
CMSSW_11_0_X_2019-10-31-2300

see notes in cms-sw/cmsdist#5321 (comment)
I also requested a backport of the root update for 10_6_X so that we have it in the long term support release

@drkovalskyi
please close this PR if you are fine with the resolution.

fabiocos · 2019-12-04T17:06:09Z

@drkovalskyi I would say that this PR can be closed at this point, please comment in case, otherwise I'll clean this from the list.

fabiocos · 2019-12-16T15:34:36Z

hold

cmsbuild · 2019-12-16T15:35:12Z

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

fabiocos · 2019-12-16T17:27:43Z

@drkovalskyi thanks

a workaround for enum to int conversion problem in pyROOT

415bea7

cmsbuild added this to the CMSSW_10_6_X milestone Apr 12, 2019

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 12, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 12, 2019

cmsbuild added tests-started and removed tests-pending labels Apr 12, 2019

cmsbuild added tests-approved and removed tests-started labels Apr 12, 2019

cmsbuild added comparison-available and removed comparison-pending labels Apr 12, 2019

slava77 reviewed Apr 13, 2019

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Aug 5, 2019

cmsbuild added tests-approved and removed tests-started labels Aug 6, 2019

cmsbuild added comparison-available and removed comparison-pending labels Aug 6, 2019

cmsbuild mentioned this pull request Oct 18, 2019

Propagate inverse beta variable to pat muons (muon POG) #28212

Merged

cmsbuild mentioned this pull request Oct 29, 2019

[HBHE] Update MahiDebugger and fix nP in NNLS #28193

Closed

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Nov 1, 2019

smuzaffar modified the milestones: CMSSW_11_0_X, CMSSW_11_1_X Dec 2, 2019

cmsbuild modified the milestones: CMSSW_11_1_X, CMSSW_11_0_X Dec 10, 2019

cmsbuild added the hold label Dec 16, 2019

drkovalskyi closed this Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[muon] a workaround for enum to int conversion problem in pyROOT #26446

[muon] a workaround for enum to int conversion problem in pyROOT #26446

drkovalskyi commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

slava77 commented Apr 12, 2019

cmsbuild commented Apr 12, 2019 •

edited

drkovalskyi commented Apr 12, 2019

slava77 commented Apr 12, 2019

drkovalskyi commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

slava77 Apr 13, 2019

drkovalskyi Apr 13, 2019

slava77 commented Apr 13, 2019

cmsbuild commented Aug 5, 2019 •

edited

cmsbuild commented Aug 6, 2019

cmsbuild commented Aug 6, 2019

cmsbuild commented Aug 6, 2019

slava77 commented Sep 3, 2019

slava77 commented Sep 6, 2019

drkovalskyi commented Sep 9, 2019

slava77 commented Nov 1, 2019

fabiocos commented Dec 4, 2019

fabiocos commented Dec 16, 2019

cmsbuild commented Dec 16, 2019

fabiocos commented Dec 16, 2019

[muon] a workaround for enum to int conversion problem in pyROOT #26446

[muon] a workaround for enum to int conversion problem in pyROOT #26446

Conversation

drkovalskyi commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

slava77 commented Apr 12, 2019

cmsbuild commented Apr 12, 2019 • edited

drkovalskyi commented Apr 12, 2019

slava77 commented Apr 12, 2019

drkovalskyi commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

cmsbuild commented Apr 12, 2019

slava77 Apr 13, 2019

Choose a reason for hiding this comment

drkovalskyi Apr 13, 2019

Choose a reason for hiding this comment

slava77 commented Apr 13, 2019

cmsbuild commented Aug 5, 2019 • edited

cmsbuild commented Aug 6, 2019

cmsbuild commented Aug 6, 2019

cmsbuild commented Aug 6, 2019

slava77 commented Sep 3, 2019

slava77 commented Sep 6, 2019

drkovalskyi commented Sep 9, 2019

slava77 commented Nov 1, 2019

fabiocos commented Dec 4, 2019

fabiocos commented Dec 16, 2019

cmsbuild commented Dec 16, 2019

fabiocos commented Dec 16, 2019

cmsbuild commented Apr 12, 2019 •

edited

cmsbuild commented Aug 5, 2019 •

edited