Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove ROOTCLING ifdefs that are no longer needed (were never?) #30748

Merged
merged 2 commits into from Jul 30, 2020

Conversation

davidlange6
Copy link
Contributor

given that cling understands c++ - changes block modules build. There is one more case in DataFormats/Common that changes class definition so we are still looking at that one.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30748/17061

  • This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30748/17063

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlange6 (David Lange) for master.

It involves the following packages:

DPGAnalysis/SiStripTools
DataFormats/CaloTowers
FWCore/MessageLogger

@perrotta, @smuzaffar, @Dr15Jones, @makortel, @cmsbuild, @slava77, @jpata, @santocch can you please review it and eventually sign? Thanks.
@echabert, @makortel, @mtosi, @pieterdavid, @robervalwalsh, @JanFSchulte, @rovere, @wddgit, @jandrea, @mmusich, @threus, @ebrondol, @venturia this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@davidlange6
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 16, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1

Tested at: 0b6087b

CMSSW: CMSSW_11_2_X_2020-07-15-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7ea8b7/8003/summary.html

I found follow errors while testing this PR

Failed tests: AddOn

  • AddOn:

I found errors in the following addon tests:

cmsDriver.py RelVal -s HLT:GRun,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run3_data_GRun --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run3 --processName=HLTRECO --filein file:RelVal_Raw_GRun_DATA.root --fileout file:RelVal_Raw_GRun_DATA_HLT_RECO.root : FAILED - time: date Thu Jul 16 18:50:22 2020-date Thu Jul 16 18:38:16 2020 s - exit: 34304

@cmsbuild
Copy link
Contributor

Comparison job queued.

@davidlange6
Copy link
Contributor Author

interesting.. so how does one go from path[21] (or 28 or 29) to cff for that filter?

@davidlange6
Copy link
Contributor Author

ok, the job log
trkPOGFilters
trkPOG_manystripclus53X
trkPOG_toomanystripclus53X

which are using Multiplicities.h

@davidlange6
Copy link
Contributor Author

I begin to reach the conclusion that this PR fixes rather than breaks.

Eg, we have a met filter that computes two multiplicities in tracker and flags
cut = cms.string("( mult2 > 20000+7mult1)")
and a second one with
cut = cms.string("(mult2>50000) && ( mult2 > 20000+7
mult1)")

In the current test workflows I looked at (136.731), these never fire. however, printouts suggest that it should

Eg, in the first few events I see
mult1 = 3396 mult1 = 36895.
--> should be both false
mult1 = 6033 mult2 = 65940
--> should be both true
mult1 = 3939 mult2 = 47651
--> one should be true the other false

anyway, its a bit deep into StringCutObjectParser for me to be confident in not missing something.

(but if so - have these filters been used in run2?)

@jpata
Copy link
Contributor

jpata commented Jul 22, 2020

Thanks for the investigations @davidlange6 ! Is it known why the behaviour of the multiplicities changes with the removal of the cling defs?

As for if these paths have been used and what the change in output entails, perhaps @mtosi @JanFSchulte know?

thanks.

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 22, 2020 via email

@mtosi
Copy link
Contributor

mtosi commented Jul 22, 2020

the cut should reflect outliers in the 2D plot which summaries the correlation between the strip and pixel clusters multiplicities https://tinyurl.com/y2fa4494

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 22, 2020 via email

@mtosi
Copy link
Contributor

mtosi commented Jul 22, 2020

the code https://cmssdt.cern.ch/dxr/CMSSW/source/DPGAnalysis/SiStripTools/plugins/ByMultiplicityEventFilter.cc#47
is an EDFilter which adds a flag w/ the result of the expression and returns the boolean itself

are you telling me that it returns True regardless of the results of the expression ?

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 22, 2020 via email

@mtosi
Copy link
Contributor

mtosi commented Jul 22, 2020

similar cut is applied in the offline reco, actually it was supposed to be applied
https://cmssdt.cern.ch/lxr/source/RecoTracker/TkSeedGenerator/python/trackerClusterCheck_cfi.py#0007 (!!!)
@VinInn can you remind me the reason for not having retuned this cut ?
I remember (old) studies which highlighted how similar selections were able to clean beam background events or events w/ issues in the detector

it is true that some LLP might find interesting some off-diagonal events

and therefore, we would really want to remove these flags from the MET filters

@VinInn
Copy link
Contributor

VinInn commented Jul 22, 2020

No clue

@Dr15Jones
Copy link
Contributor

I don’t, no. I’m not familiar with how the cut parser relies on dictionaries @Dr15Jones maybe does? The class structure here is quite different with and without the defs (which means the dictionary looked different than the class object in memory)

The cut parser uses the dictionary to associate the names of functions (which are in the cut) to the actual functions to be called. If the use of #define lead to a one definition rule violation then we are in compiler undefined territory.

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 22, 2020 via email

@jpata
Copy link
Contributor

jpata commented Jul 23, 2020

@davidlange6 thanks for that summary. For completeness, could you drop a hint here how you traced the issue exactly to the modules

trkPOGFilters
trkPOG_manystripclus53X
trkPOG_toomanystripclus53X

When I produce the job log with process.options.wantSummary=True, I only find the TrigReports for the generic trigger paths, not the granular triggers/cffs, so I must be missing something. Thanks!

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 23, 2020 via email

@jpata
Copy link
Contributor

jpata commented Jul 23, 2020

Per David's quick investigations above (thx for the tips!), I also confirm that this PR seems to change the behaviour in trkPOGFilters, trkPOG_manystripclus53X, trkPOG_toomanystripclus53X in a number of investigated events.

I observe on the workflow 136.731:

Begin processing the 4th record. Run 274199, Event 40112763, LumiSection 21
...
  this PR                                                  |  base release

  name=(mult2>50000) && ( mult2 > 20000+7*mult1)           |  name=(mult2>50000) && ( mult2 > 20000+7*mult1)
  mult2=47651                                              |  mult2=0
  value=0                                                  |  value=0
  name=( mult2 > 20000+7*mult1)                            |  name=( mult2 > 20000+7*mult1)
  mult1=3939                                               |  mult1=3939
  mult2=47651                                              |  mult2=0
  value=1                                                  |  value=0

(full logs in /afs/cern.ch/user/j/jpata/public/30748/*.log)

which seems rather like a different input (mult2=0 in the base release) than a different behaviour of the cut parser. Perhaps I miss something.

@davidlange6
Copy link
Contributor Author

hi @jpata - I think you are correct - I must have miss understand the log. I gather these multiplicities are subdet=5 (aka, Pixel) and subdet=0 (aka, Strip) with the number of clusters from clusterSummaryProducer as the metric. Is 0 expected for one of these (mult2 is nominally strips if I got it right)?

@jpata
Copy link
Contributor

jpata commented Jul 24, 2020

When I print out the raw data in https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackerCommon/interface/ClusterSummary.h#L109 the behaviour in the base release seems incorrect as you point out. The multiplicity vectors contain valid numbers, however, the mult2() function returns 0. I have not been able to understand the underlying reason, but I suspect it's something along the lines of Chris's suggestion of undefined behaviour.

Now the question is how to proceed. Would it be viable to merge this PR which fixes a bug but changes previous behaviour?
As I understand from Mia above, for the TRK pog, these bugged filters could be removed. How about JetMET @ahinzmann @lathomas @kirschen (sorry I'm not sure who is the reco contact)?

@jpata
Copy link
Contributor

jpata commented Jul 28, 2020

The Multiplicities data structure contains physical and valid data up until the call to StringCutObjectSelector::operator(), at which point mult2=0 and the stack trace as recorded by gdb becomes corrupted.

This further points to the fact that the base release has undefined behaviour, which could be due to conflicting class definitions between the dictionaries as interpreted by cling and compiled by gcc (as suggested by Chris).

The behaviour of ByClusterSummaryMultiplicityPairEventFilter with this PR included seems correct, i.e. the Multiplicities data structures contain reasonable values throughout and the StringCutObjectSelector returns reasonable outputs. I would say this PR can be merged, with the understanding that previously incorrect behaviour is changed and corrected.

There seems to be one other spot in cmssw where #ifdef __ROOTCLING__ is used to substantially change the class: https://github.com/cms-sw/cmssw/blob/master/DataFormats/Common/interface/DetSetVectorNew.h#L13.

@davidlange6
Copy link
Contributor Author

davidlange6 commented Jul 28, 2020 via email

@jpata
Copy link
Contributor

jpata commented Jul 29, 2020

+1

  • fixes previously buggy result in ByClusterMultiplicityFilter that changes reco output in some trigger paths: trkPOGFilters, trkPOG_manystripclus53X, trkPOG_toomanystripclus53X
  • code looks good and is otherwise technical, i.e. no algorithmic changes

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 9e7edae into cms-sw:master Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants