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

Changing abs(eta) cut for tracks in ALCARECOEcalESAlign_cff.py #40067

Merged
merged 1 commit into from Dec 17, 2022

Conversation

rekkhan
Copy link
Contributor

@rekkhan rekkhan commented Nov 15, 2022

Loosen the abs(eta) cut for the tracks

  • lower abs(eta) change from 1.7 to 1.65.
  • remove the higher abs(eta) cut

PR description:

Change the abs(eta) cut of the track.

  • Lower cut change to 1.65
  • Remove higher cut

PR validation:

Current abs(eta) cut decrease statistic in high eta region of the Preshower. Loosening the eta cut is proposed to increase the hits statistic. Hits occupancy issue can be seen in the plots in my cernbox[1]

[1]https://cernbox.cern.ch/s/FmTTfZvdBRXxKRW

Loosen the abs(eta) cut for the tracks
- lower abs(eta) change from 1.7 to 1.65.
- remove the higher abs(eta) cut
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40067/33047

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rekkhan (Long Hoa) for master.

It involves the following packages:

  • Calibration/EcalAlCaRecoProducers (alca)

@malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@rchatter, @tocheng, @argiro, @thomreis, @simonepigazzini, @mmusich this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@thomreis
Copy link
Contributor

please test

@francescobrivio
Copy link
Contributor

Hi @rekkhan, @thomreis,

I have two questions:

  • Just for my understanding what is the rationale behind removing completely the higher eta cut? Does this mean you will accept also tracks outside the tracker acceptance (i.e. abs(eta)>2.5)?
  • do you have an estimate of the size increaze of the alcareco with these changes?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ba8825/29011/summary.html
COMMIT: b3a51c4
CMSSW: CMSSW_12_6_X_2022-11-15-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40067/29011/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3417074
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3417049
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 206 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 3 / 46 workflows

@mmusich
Copy link
Contributor

mmusich commented Nov 15, 2022

Does this mean you will accept also tracks outside the tracker acceptance (i.e. abs(eta)>2.5)?

Not sure what it even means to have tracks outside of the Tracker acceptance, but FYI it is possible with the phase1 pixel detector to have tracks made exclusively out of pixel hits up to |eta|<3.

@tvami
Copy link
Contributor

tvami commented Nov 15, 2022

please test

@thomreis I dont think the standard relvals run this ALCARECO. Which relvals run this? Should we just run the one with allForExpress and allForPrompt?

@tvami
Copy link
Contributor

tvami commented Nov 15, 2022

@rekkhan please also be more specific in the PR title, thanks!

@francescobrivio
Copy link
Contributor

Does this mean you will accept also tracks outside the tracker acceptance (i.e. abs(eta)>2.5)?

Not sure what it even means to have tracks outside of the Tracker acceptance, but FYI it is possible with the phase1 pixel detector to have tracks made exclusively out of pixel hits up to |eta|<3.

yup sorry that didn't mean anything 😅
I meant to ask what was the rationale behind releasing completely the upper bound on eta, or more precisely I wanted to understand why the previous abs(eta)<2.3 selection is no longer needed?

@thomreis
Copy link
Contributor

please test

@thomreis I dont think the standard relvals run this ALCARECO. Which relvals run this? Should we just run the one with allForExpress and allForPrompt?

Hi @tvami, runTheMatrix.py --showMatrix --extended -l limited | grep EcalESAlign finds several WFs that contain EcalESAlign so I think the changes should be covered by the tests.

@rekkhan rekkhan changed the title Update ALCARECOEcalESAlign_cff.py Changing abs(eta) cut for tracks in ALCARECOEcalESAlign_cff.py Nov 17, 2022
@rekkhan
Copy link
Contributor Author

rekkhan commented Nov 17, 2022

Does this mean you will accept also tracks outside the tracker acceptance (i.e. abs(eta)>2.5)?

Not sure what it even means to have tracks outside of the Tracker acceptance, but FYI it is possible with the phase1 pixel detector to have tracks made exclusively out of pixel hits up to |eta|<3.

yup sorry that didn't mean anything sweat_smile I meant to ask what was the rationale behind releasing completely the upper bound on eta, or more precisely I wanted to understand why the previous abs(eta)<2.3 selection is no longer needed?

Hi @francescobrivio
We need tracks at higher eta to do the ES MIP calibration as the preshower cover up to 2.65 in abs(eta). The abs(eta) cut at 2.3 reduces the number of tracks required to calibrate the sensors in higher abs(eta) region, so we want to remove that cut.

@tvami
Copy link
Contributor

tvami commented Nov 17, 2022

sorry was this question

do you have an estimate of the size increaze of the alcareco with these changes?

answered?

@rekkhan
Copy link
Contributor Author

rekkhan commented Nov 17, 2022

sorry was this question

do you have an estimate of the size increaze of the alcareco with these changes?

answered?

Sorry, we are still testing the effect of the cut on the file size. I will report as soon as possible.

@cmsbuild cmsbuild modified the milestones: CMSSW_12_6_X, CMSSW_13_0_X Nov 24, 2022
@ChrisMisan
Copy link
Contributor

@rekkhan is there any update regarding this pr?

@rekkhan
Copy link
Contributor Author

rekkhan commented Dec 7, 2022

@rekkhan is there any update regarding this pr?

Sorry for the delay but I still haven't had the result yet. The colleague in my group who know how to run the test is not available at the moment.
As I'm not sure if he will be able to run the test soon, could you please point me to some tutorial/instruction how to run the test so I can produce the result by myself?

@ChrisMisan
Copy link
Contributor

@rekkhan is there any update regarding this pr?

Sorry for the delay but I still haven't had the result yet. The colleague in my group who know how to run the test is not available at the moment. As I'm not sure if he will be able to run the test soon, could you please point me to some tutorial/instruction how to run the test so I can produce the result by myself?

HI @rekkhan, if I understood correctly you'd like to test for the size increase? If so, this wiki page might be useful: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuidePerformanceSuite

@rekkhan
Copy link
Contributor Author

rekkhan commented Dec 8, 2022

Hi @ChrisMisan
Thank you, I'm reading the twiki page.

@ChrisMisan
Copy link
Contributor

@rekkhan we've found a script to help with size comparison. Can you send a list of wfs that'd like to run to test it?

@tvami
Copy link
Contributor

tvami commented Dec 13, 2022

@ChrisMisan you can run runTheMatrix.py --showMatrix --extended -l limited | grep EcalESAlign to help to find the wf-s that create this ALCARECO

@rekkhan
Copy link
Contributor Author

rekkhan commented Dec 14, 2022

Thank you @tvami , I cannot run the performance suite from the twiki due to the unset HOST variable.
From running your command line, I get:

[3]: cmsDriver.py step3  --conditions auto:run2_data_relval -s RAW2DIGI,L1Reco,RECO,SKIM:SinglePhotonJetPlusHOFilter+EXOMONOPOLE,EI,PAT,ALCA:SiStripCalZeroBias+SiStripCalMinBias+TkAlMinBias+EcalESAlign,DQM:@standardDQM+@ExtraHLT+@miniAODDQM --runUnscheduled  --process reRECO --data  --era Run2_2016_HIPM --eventcontent RECO,MINIAOD,DQM --hltProcess reHLT --scenario pp --datatier RECO,MINIAOD,DQMIO --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2016 -n 100 
[3]: cmsDriver.py step3  --conditions auto:run2_data -s RAW2DIGI,L1Reco,RECO,SKIM:SinglePhotonJetPlusHOFilter+EXOMONOPOLE,EI,PAT,ALCA:SiStripCalZeroBias+SiStripCalMinBias+TkAlMinBias+EcalESAlign,DQM:@standardDQM+@ExtraHLT+@miniAODDQM --runUnscheduled  --process reRECO --data  --era Run2_2017 --eventcontent RECO,MINIAOD,DQM --hltProcess reHLT --scenario pp --datatier RECO,MINIAOD,DQMIO --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2017 -n 100 
[3]: cmsDriver.py step3  --conditions auto:run2_data -s RAW2DIGI,L1Reco,RECO,SKIM:ZElectron+SinglePhotonJetPlusHOFilter+EXOMONOPOLE,EI,PAT,ALCA:SiStripCalZeroBias+SiStripCalMinBias+SiStripCalSmallBiasScan+TkAlMinBias+EcalESAlign,DQM:@standardDQM+@ExtraHLT+@miniAODDQM+@L1TEgamma --runUnscheduled  --process reRECO --data  --era Run2_2018 --eventcontent RECO,MINIAOD,DQM --hltProcess reHLT --scenario pp --datatier RECO,MINIAOD,DQMIO --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2018 -n 100 
[5]: cmsDriver.py step5  --conditions auto:phase1_2017_realistic -n 10 --era Run2_2017 --eventcontent ALCARECO --filein file:step3.root -s ALCA:SiPixelCalSingleMuon+TkAlMuonIsolated+TkAlMinBias+MuAlOverlaps+EcalESAlign+TkAlZMuMu+HcalCalHBHEMuonFilter+TkAlUpsilonMuMu+TkAlJpsiMuMu+SiStripCalMinBias --datatier ALCARECO --geometry DB:Extended
[5]: cmsDriver.py step5  --conditions auto:phase1_2017_realistic -n 10 --era Run2_2017 --eventcontent ALCARECO --filein file:step3.root -s ALCA:SiPixelCalSingleMuon+TkAlMuonIsolated+TkAlMinBias+MuAlOverlaps+EcalESAlign+TkAlZMuMu+HcalCalHBHEMuonFilter+TkAlUpsilonMuMu+TkAlJpsiMuMu+SiStripCalMinBias --datatier ALCARECO --geometry DB:Extended
[5]: cmsDriver.py step5  --conditions auto:phase1_2018_realistic -n 10 --era Run2_2018 --eventcontent ALCARECO --filein file:step3.root -s ALCA:SiPixelCalSingleMuon+TkAlMuonIsolated+TkAlMinBias+MuAlOverlaps+EcalESAlign+TkAlZMuMu+HcalCalHBHEMuonFilter+TkAlUpsilonMuMu+TkAlJpsiMuMu+SiStripCalMinBias --datatier ALCARECO --geometry DB:Extended
[5]: cmsDriver.py step5  --conditions auto:phase1_2021_realistic -n 10 --era Run3 --eventcontent ALCARECO --filein file:step3.root -s ALCA:SiPixelCalSingleMuon+TkAlMuonIsolated+TkAlMinBias+MuAlOverlaps+EcalESAlign+TkAlZMuMu+HcalCalHBHEMuonFilter+TkAlUpsilonMuMu+TkAlJpsiMuMu+SiStripCalMinBias --datatier ALCARECO --geometry DB:Extended

@ChrisMisan
Copy link
Contributor

@tvami running 1001.3 didn't show any significant changes to the event sizes.

@tvami
Copy link
Contributor

tvami commented Dec 16, 2022

+alca

  • see comment from Chris

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

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 774e317 into cms-sw:master Dec 17, 2022
@rekkhan
Copy link
Contributor Author

rekkhan commented Dec 19, 2022

Thank you very much for helping me

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

8 participants