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

[12_0_X] HCAL: switch off auxiliary M3 reconstruction fit for Run3 #35807

Merged
merged 3 commits into from Oct 26, 2021

Conversation

abdoulline
Copy link

@abdoulline abdoulline commented Oct 24, 2021

PR description:

M3 signal fit is an auxiliary one (MAHI is default), but it stays activated in HCAL Offline reconstruction (while swithed off since long time for HLT) and it wasn't explcitly looked at in the context of Run3 preparations and it's not used by any downstream consumers.
HCAL is now discussing to possibly use corresponding HBHERecHit member (auxEnergy) for saving next-to-trigger BX (SOI+1 in HCAL notations) energy from MAHI for LLP studies.

NB: this PR serves as a workaround for recently reported issue #35785
while (in parallel) HCAL DPG figures out what went wrong with M3 as reported here:
https://hypernews.cern.ch/HyperNews/CMS/get/tier0-Ops/2294.html

PR validation:

runTheMatrix.py -l limited

This PR is a backport of

#35806

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 24, 2021

A new Pull Request was created by @abdoulline (Salavat Abdullin) for CMSSW_12_0_X.

It involves the following packages:

  • RecoEgamma/EgammaHLTProducers (hlt)
  • RecoLocalCalo/HcalRecProducers (reconstruction)

@jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @HuguesBrun, @calderona, @silviodonato, @jainshilpi, @Fedespring, @lgray, @apsallid, @sscruz, @bsunanda, @afiqaize, @wrtabb, @mariadalfonso, @ram1123, @varuns23, @cericeci, @sobhatta this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@abdoulline abdoulline changed the title [12_0_X] HCAL: auxiliary M3 reconstruction fit switching off for Run3 [12_0_X] HCAL: switch off auxiliary M3 reconstruction fit for Run3 Oct 24, 2021
@missirol
Copy link
Contributor

backport of #35806

@missirol
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6adf3/19873/summary.html
COMMIT: 0ba73bf
CMSSW: CMSSW_12_0_X_2021-10-24-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35807/19873/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: 68 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 124
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2998417
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@@ -2654,3 +2654,6 @@
PixelCPE = cms.string('PixelCPEGeneric'),
StripCPE = cms.string('StripCPEfromTrackAngle')
)

from Configuration.Eras.Modifier_run3_common_cff import run3_common
run3_common.toModify(hltHbhereco, algorithm = dict(useM3 = False))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HLT does not use Eras and anway has useM3 = cms.bool( False ) so this is not needed for HLT.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Martin-Grunewald thank you for calrification, I (naively) thought this might be a kind of standalone config used for some purposes. I've removed this modification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I see that that file (PhaseII) actually sets useM3 = True which seems nonsensical but then it has to be switched to false after all. Apologies for the confusion!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Martin-Grunewald just to avoid any (more) confusion: do you suggest to bring the (just removed) customization back ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but also I want Andrea Bocci @fwyzard to clarify why in that Phase-II file useM3 = True is used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay - this configuration is not used by the actual Phase-2 HLT menu (which anyway is not yet in 12.0.x/12.1.x...).
I don't know where it comes from, or if it used for anything - I guess only E/Gamma people can comment here.

@cmsbuild
Copy link
Contributor

Pull request #35807 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #35807 was updated. @jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Oct 25, 2021

@cmsbuild , please test

@abdoulline
Copy link
Author

@rappoccio thanks for the config file! It's all OK now (see above).

@slava77
Copy link
Contributor

slava77 commented Oct 25, 2021

+reconstruction

for #35807 2eaeae3

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show differences in M3 plots in HBHE validation
    • there should probably be a cleanup of the DQM at some point to skip/remove these plots @cms-sw/dqm-l2

I'm signing this a bit out of order (before #35806, just in case a release is needed sooner in 12_0_X)

@missirol
Copy link
Contributor

+hlt

  • the change in RecoEgamma/EgammaHLTProducers does not affect any workflow
    (and HLT already uses useM3 = False)

  • HLTEgPhaseIITestSequence_cff was added in HGCal ID variables #32519 only for testing purposes
    (it could arguably be removed altogether; otoh, there is currently no plan to include the current HLT Phase-2 menu in 12.0.X)

@cmsbuild
Copy link
Contributor

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

@abdoulline
Copy link
Author

@slava77 indeed, we plan to refill these DQM plots ("M3" flavor, now become empty) with what will be "auxEnergy" Run3 replacement, most probably SOI+1 MAHI energy.

@tvami
Copy link
Contributor

tvami commented Oct 25, 2021

urgent

  • this should be in 12_0_3_patch1 since it resolves the issue for PromptReco_Run345915_ZeroBias19

@davidlange6
Copy link
Contributor

one doesn't get to change things by hand at the tier0 - someone checked also a config created from configuration dataprocessing with this change?

@abdoulline
Copy link
Author

abdoulline commented Oct 26, 2021

@davidlange6
I have only indirect answer(s):

(1) HCAL-related "SwitchProducerCUDA" uses a clone of properly modified config:

(2) I've dumped a simple Run3 MC config (with this PR in 12_0_3) and can see that there are only useM3 = False instances...

@rappoccio
Copy link
Contributor

Hi @davidlange6 yes, understood, @germanfgv is working on how to create a new config.

@germanfgv
Copy link
Contributor

@davidlange6 I'm actually not sure on how we can produce the PSet using this patch. Our usual procedure just loads the python packages from /cvmfs/. I have generated the process_funcArgs.json file[1], but I'm not sure how to procede.

[1] /afs/cern.ch/user/c/cmst0/public/PausedJobs/PilotBeam/Replay/job_3665/patch35807/job/WMTaskSpace/cmsRun1

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 26, 2021 via email

@perrotta
Copy link
Contributor

+1

  • Let have this switched off also in 12_0, even without waiting for the master IB which contains the same update, to speed up the build of 12_0_3_patch1: the update doesn't look harmful anyhow

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