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

Don't recluster jets in HI miniAOD for Run 3 #38711

Merged
merged 3 commits into from Jul 26, 2022

Conversation

mandrenguyen
Copy link
Contributor

PR description:

Jets were being reclustered in the PAT step for heavy ions, as the jet algorithm had been updated between the time the last data were reconstructed (2018), and the first miniAOD campaign (2020).
For the upcoming data (era=pp_on_PbPb_run3), the miniAOD will be done concurrently with the reconstruction such that this is no longer necessary.
This PR removes the reclustering for that era, as well as legacy b-tagging, which is no longer needed. DeepCSV is activated in its place. Finally, an electron fix for 2018 reMiniAOD is also removed that is no longer necessary.

PR validation:

Tested 159, 140.5611 and 158.01

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38711/31009

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @jdolen, @JyothsnaKomaragiri, @ahinzmann, @schoef, @emilbols, @jdamgov, @mbluj, @nhanvtran, @gkasieczka, @hatakeyamak, @gpetruc, @azotz, @mariadalfonso, @demuller, @andrzejnovak, @seemasharmafnal, @mmarionncern this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@clacaputo
Copy link
Contributor

Hi @mandrenguyen did you do any performance comparisons in 159? Naively, I'd expect a reduction in timing

@clacaputo
Copy link
Contributor

@cmsbuild please test

@mandrenguyen
Copy link
Contributor Author

Hi @clacaputo We should see a large speedup of PAT, but that will not be super evident when it's run concurrently with reco (as in 159). The best place to see this would be in a stand-alone re-miniAOD workflow like 158.01. That wf is for 2018 though, where the reclustering is necessary, unless the 2018 PbPb data is re-reco'd. For Run 3, we don't yet have a re-miniAOD MC RelVal wf. How about I add a 159.01 to this PR for that purpose?

For reasons that are a bit complicated to explain here, we plan to produce miniAOD at the HI T2 (not together with prompt reco), such that the PAT timing is an important consideration.

Note also that without this PR, Run 3 HI re-miniAOD (i.e., not concurrently with reco) doesn't even run due to a collection label issue. I was actually trying to solve that issue when I realized that we should really remove the reclustering.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0edc59/26242/summary.html
COMMIT: c1d3d15
CMSSW: CMSSW_12_5_X_2022-07-13-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38711/26242/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: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3653966
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3653942
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

How about I add a 159.01 to this PR for that purpose?

I think it's a good idea

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38711/31059

@srimanob
Copy link
Contributor

Hi @mandrenguyen
I'm not I understand the strategy of the PR test here. The goal seems to be MiniAOD that comes together with reconstruction, not reminiaod. Why do we have reminiaod here for the PR test, instead of updating HI Run3 workflow? Do I miss something from the PR description? Thanks.

@mandrenguyen
Copy link
Contributor Author

Hi @srimanob 159 already has miniAOD as part of the reconstruction. The issue is that it miniAOD was crashing when not run as part of the reco (i.e., "re-miniAOD"). While fixing that I noticed that we no longer need to recluster the jets, which was the most time consuming part of the 2018 re-miniAOD (wf 158.01). We do plan to run the miniAOD separately from reco in 2022, and the speed up from not reclustering the jets will be important. Does that clear it up?

@srimanob
Copy link
Contributor

OK, this is more clear now. Thanks @mandrenguyen

Just my 2 cents:
(1) I don't see 159.01 is triggered in the PR test. Should we do it here?

(2) One thing that HI workflow does not reflect is the real production. I assume in the real situation, you will start from AOD, not GEN-SIM-RECO. I don't expect any issues, i.e. broken of MiniAOD when start from AOD. But it is quite simple to make it happen also in relvals. I let's you and PdmV decide on this. I am happy to sign this PR anyways (just after the trigger of 159.01 if needed).

@mandrenguyen
Copy link
Contributor Author

Yes, someone please trigger 159.1 :-)

Regarding AOD as input, there is currently not a suitable dataset produced in relval. When the real 2022 PbPb data arrives, we will add a workflow like 140.5611, which tests AOD --> mAOD for 2018 data. In the meantime, I'm fairly confident that if it works from RECO, it will work from AOD, but we will test this manually prior to the data taking.

@clacaputo
Copy link
Contributor

test parameters:

  • workflow = 159.1

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0edc59/26322/summary.html
COMMIT: 3f512ee
CMSSW: CMSSW_12_5_X_2022-07-18-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38711/26322/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-0edc59/159.1_QCD_Pt_80_120_14_HI_2021+QCD_Pt_80_120_14_HI_2021+DIGIHI2021PPRECO+RECOHI2021PPRECO+HARVESTHI2021PPRECO

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3760699
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3760669
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 45 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

+Upgrade

@clacaputo
Copy link
Contributor

+reconstruction

  • jets reclustering disabled for Run3 HI miniAOD
  • new wf 159.01 added for checking HI re-miniAOD performances

@bbilin
Copy link
Contributor

bbilin commented Jul 26, 2022

+pdmv

@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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

  • I assume that the pairDiscriVector being empty for slimmedJets in wf 312 is an expected consequence of this PR. While we merge it for the nightly IB, please confirm

@cmsbuild cmsbuild merged commit 61a96f0 into cms-sw:master Jul 26, 2022
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

6 participants