Add more calorimeter energy fractions to PackedCandidates

[steggema]

PR description:

This PR adds two new data members to PackedCandidates to be able to save two calorimeter energy fractions (one related to ECAL over HCAL energy, the other the ratio of calorimetric energy to overall candidate energy so that absolute HCAL energy fractions as well as track-ECAL energy ratios can be calculated) to charged PF candidates in MiniAOD. The additional information is the main missing information to be able to run algorithms to reject electrons and/or muons misreconstructed as taus on taus reconstructed from MiniAOD information only (see #22594).

The PR is expected to increase the size of the MiniAOD output by 1.1%, as measured on a 10000-event TTW file from the Autumn 18 production. The size of the pat::PackedCandidates branch is expected to increase from

Branch Name | Average Uncompressed Size (Bytes/Event) | Average Compressed Size (Bytes/Event)
patPackedCandidates_packedPFCandidates__PAT. 141015 20723.3

to

patPackedCandidates_packedPFCandidates__PAT. 145227 21500.8

The proposed implementation adds two new data members so that we end up with two data members that hold "raw" information and two that hold regular calibrated information. The reason is that JetMET currently also uses the regular energy fraction data member to save raw energy fractions for isolated charged hadrons so that they can run the charged hadron calibration on MiniAOD events. I found no traces of usage in CMSSW (except for b-tagging, who use the energy fractions saved for neutrals as was the original purpose of these members) and hence propose to move the JetMET content to the raw data mebers so we can save the new information in a logically consistent way in the existing data members (@ahinzmann @zdemirag this may affect you).

@mverzett @kskovpen As far as I understand the BTV code, the DNN-based b-taggers (who seem to be the only customers in CMSSW) only use the information saved for neutrals, which are unchanged.

This is to address #26406 (see also the discussion therein). @peruzzim @rmanzoni @mbluj @slava77 @fgolf @swozniewski

PR validation:

Basic PR tests have been performed.

if this PR is a backport please specify the original PR:

N/A

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26506/9368

• This PR adds an extra 48KB to repository

[cmsbuild]

A new Pull Request was created by @steggema (Jan Steggemann) for master.

It involves the following packages:

DataFormats/PatCandidates
PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @hatakeyamak, @rappoccio, @HeinerTholen, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @jdolen, @drkovalskyi, @ferencek, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @cbernet, @gpetruc, @mariadalfonso, @pvmulder 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]

@cmsbuild please test

[ahinzmann]

Just to make sure I read the description and code correctly. This will keep saving for all isolated charged hadrons, the raw energy fractions, just under a different (+better) name, right?

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34298/console Started: 2019/04/23 14:21

[slava77]

assign xpog
to sign off on the size increase and perhaps double-check the code changes as well

[cmsbuild]

New categories assigned: xpog

@fgolf,@peruzzim you have been requested to review this Pull request/Issue and eventually sign? Thanks

[steggema]

Just to make sure I read the description and code correctly. This will keep saving for all isolated charged hadrons, the raw energy fractions, just under a different (+better) name, right?

Yes, this is the intention.

[cmsbuild]

-1

Tested at: ebfef1b

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26506/34298/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

• RelVals:

When I ran the RelVals I found an error in the following workflows:
1000.0 step3

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step3_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVDSIPIXELCALRUN1+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVDSIPIXELCALRUN1+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5.log

[cmsbuild]

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

[slava77]

@davidlange6 @fabiocos
is there a way to add braces in clang-format for the cases of multi-line break-up of an if/else?
The result looks like a bit more dangerous pattern where and else can get misplaced

[davidlange6]

no - clang-format changes whitespace only. There is a clang-tidy option to add braces for ifs/fors/whiles that has been tried but so far doesn't work fully in cmssw (afaik).

[slava77]

@davidlange6 @fabiocos
this is apparently breaking the commented out code

[slava77]

@davidlange6 @fabiocos
for this and similar cases is there a way to make clang-format not break up similarly looking lines in a different way?
it seems also unnecessary to have .encode().empty() broken up to two pretty short lines. Was it intended in the configuration

[davidlange6]

there are not an infinite number of options, no. This presumably happens because of the greater number of characters in the second line.

One can try by hand just to have a line break before .encode() on both lines (but that also may get changed by clang-format)

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 24 differences found in the comparisons
• DQMHistoTests: Total files compared: 33
• DQMHistoTests: Total histograms compared: 3211964
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3211759
• DQMHistoTests: Total skipped: 204
• 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]

+1

for #26506 3120396

• code changes are in line with the PR description and the follow up review. Some effort was made to preserve the old treatment of hcalFraction in the deep jet/b-tagging code.
• jenkins tests pass and comparisons with the baseline show differences only in the hcalFraction values, which are now also filled for most of the charged hadrons.
  • deep jet/b-tagging behavior is unchanged

[peruzzim]

+xpog

[perrotta]

@davidlange6 here and in other recent PRs (see #26606 for another example) line breaking as implemented by clang mostly results in less easily readable code; moreover, it complicates the reviews quite a lot. Given my recent experiences I would simply avoid forcing all line breaks in the code checks resolution, unless some more clever setting can be found for them.

[davidlange6]

I guess the review/discussions over the last year disagrees with that conclusion. But you are welcome to make a specific proposal to the core group.

…

On May 7, 2019, at 10:14 AM, perrotta ***@***.***> wrote: @davidlange6 here and in other recent PRs (see #26606 for another example) line breaking as implemented by clang mostly results in less easily readable code; moreover, it complicates the reviews quite a lot. Given my recent experiences I would simply avoid forcing all line breaks in the code checks resolution, unless some more clever setting can be found for them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[fabiocos]

+1

[fabiocos]

merge

[santocch]

+1

[cmsbuild]

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 be automatically merged.