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

Change the default phase 2 PF reconstruction for endcap from simPF to pfTICL #32766

Merged
merged 2 commits into from Feb 10, 2021

Conversation

hatakeyamak
Copy link
Contributor

@hatakeyamak hatakeyamak commented Jan 29, 2021

PR description:

This PR will change the default phase 2 PF reconstruction for endcap from simPF to pfTICL. This is based on the agreed plan between the PF group and HGCAL DPG that we will make this switch when #32291 is integrated (which happened a couple of days ago). The simPFProducer is still run for now, but not injected to the PF candidate list.

PR validation:

I ran this PR with the default setting (pfTICL) and also with --customise RecoParticleFlow/Configuration/RecoParticleFlow_cff.replaceTICLwithSimPF, and compared PF candidate outputs (actually packedCandidates) with those without this PR with the default (SimPF) and pfTICL injected. The test was done with 23234.0_TTbar 2026D49 without PU. I confirmed identical results when pfTICL or simPF is used for both without and with this PR.

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is not a backport.

@bendavid @rovere @felicepantaleo @jnsandhya @zeratul87

@cmsbuild cmsbuild added this to the CMSSW_11_3_X milestone Jan 29, 2021
@hatakeyamak hatakeyamak changed the title Change the default phase 2 reconstruction for endcap from simPF to pfTICL Change the default phase 2 PF reconstruction for endcap from simPF to pfTICL Jan 29, 2021
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32766/20927

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

RecoHGCal/TICL
RecoParticleFlow/Configuration
RecoParticleFlow/PFProducer

@perrotta, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata can you please review it and eventually sign? Thanks.
@mmarionncern, @felicepantaleo, @rovere, @lgray, @apsallid, @sobhatta, @clelange, @lecriste, @cbernet, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@srimanob
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0416e2/12594/summary.html
COMMIT: 914f1a9
CMSSW: CMSSW_11_3_X_2021-01-28-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32766/12594/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3700 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716596
  • DQMHistoTests: Total failures: 14353
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2702221
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2021

enable profiling

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0416e2/12600/summary.html
COMMIT: 914f1a9
CMSSW: CMSSW_11_3_X_2021-01-28-2300/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32766/12600/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3700 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716596
  • DQMHistoTests: Total failures: 14353
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2702221
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@jpata
Copy link
Contributor

jpata commented Feb 2, 2021

Some remarks:

  1. significantly fewer PFCandidates produced in the range 1.8<|eta|<3.0: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-01-28-2300+0416e2/41030/validateJR/all_OldVSNew_TTbar14TeV2026D49wf23234p0/all_OldVSNew_TTbar14TeV2026D49wf23234p0c_recoPFCandidates_particleFlow__RECO_obj_eta.png, consequently downstream quantities also change. @hatakeyamak can you confirm this is expected from the change?
  2. only 2026D60 and 2026D49 workflows are affected
  3. miniAOD size in 23434.21 goes up by ~3%, driven by patIsolatedTracks_isolatedTracks__PAT: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0416e2/12600/profiling/23434.21/products_miniAOD_sizes_compare_23434.21.txt, not clear how this is consistent with 1)
  4. AOD size doesn't change
  5. code changes LGTM

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 2, 2021

Putting some plots here for posterity:

eta:
image

pt (showing that the decrease is mostly at low pt):
image

@hatakeyamak
Copy link
Contributor Author

hatakeyamak commented Feb 2, 2021

@jpata @kpedro88 thank you.

I certainly see smaller number of PF candidates in my private test with TICL. I believe the changes are mostly coming from lowpt neutrals which are sensitive to reconstruction details (that's what I see in my private test). @kpedro88 can the above plots be made for charged candidates and neutral candidates separately?

miniAOD size in 23434.21 goes up by ~3%, driven by patIsolatedTracks_isolatedTracks__PAT: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0416e2/12600/profiling/23434.21/products_miniAOD_sizes_compare_23434.21.txt, not clear how this is consistent with 1)

@jpata I think this is consistent with the fact that there a little reduced neutrals, which lead to somewhat increased number of isolated tracks.

I wonder @rovere has any additional comments/insight to offer.

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 2, 2021

Here are the available plots generated for the comparison (of course, one can also download the output files from the matrix tests and make any plots):

pt (charged hadron)

image

pt (electron)

image

pt (photon)

image

pt (neutral hadron)

image

pt (HF hadron)

image

The changes are indeed concentrated in the photon and neutral hadron categories.

@jpata
Copy link
Contributor

jpata commented Feb 9, 2021

test parameters:

  • enable_tests = none

@jpata
Copy link
Contributor

jpata commented Feb 9, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0416e2/12775/summary.html
COMMIT: 25bf846
CMSSW: CMSSW_11_3_X_2021-02-08-2300/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32766/12775/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3906 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2751765
  • DQMHistoTests: Total failures: 13939
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2737803
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@smuzaffar
Copy link
Contributor

smuzaffar commented Feb 9, 2021

Profiling job again timeed out after 6 hours. For now I have explicitly marked the profiling status to pass so that you can get the pr test results

@smuzaffar
Copy link
Contributor

@jpata , #32766 (comment) was the correct way to disable the profiling but there was a bug due to which bot was not overriding the #32766 (comment) request. This has been fixed now

@jpata
Copy link
Contributor

jpata commented Feb 10, 2021

+reconstruction

  • changes the default phase 2 PF reconstruction for endcap from simPF to pfTICL
  • reco changes observed: less neutrals and photons, changes to jet resolution, these are all expected
  • miniAOD size in 23434.21 goes up by ~3%
  • code changes are OK

@srimanob
Copy link
Contributor

+Upgrade

OK for the code. But not clear on validation plan. What is the validation plan?
Should validation based on the same RAW, and compare between simPF and pfTICL with pre-release? High statistics?

@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)

@hatakeyamak
Copy link
Contributor Author

hatakeyamak commented Feb 10, 2021

OK for the code. But not clear on validation plan. What is the validation plan?
Should validation based on the same RAW, and compare between simPF and pfTICL with pre-release? High statistics?

If this one goes into 11_3_0_pre3, the comparison w.r.t. pre2 will become a good validation for this change.

On Monday's PPD coordination, we requested:

  • if possible, flat QCD for phase 2: increase statistics from usual 50k to 100k events (both noPIU and PU)
  • add VBF H invisible with 200k events

for 11_3_0_pre3 and I think PdmV is looking into it. The same RAW (i.e. RECO only) will be helpful.

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8377aea into cms-sw:master Feb 10, 2021
@gartung
Copy link
Member

gartung commented Feb 10, 2021

Profiling job again timeed out after 6 hours. For now I have explicitly marked the profiling status to pass so that you can get the pr test results

@smuzaffar I added a timeout of 4 hours to the igprof commands in the profiling scripts.

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

10 participants