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

Phase2 improved conditions and ntuple maker update #32220

Merged

Conversation

OzAmram
Copy link
Contributor

@OzAmram OzAmram commented Nov 20, 2020

This PR updates the phase2 template and genError objects to have extended angle coverage. This was needed in order to correct a previous issue where there were a good number of hits outside their angle space and lead to poor resolution.
Results validating these condition changes were shown at the phase 2 simulation meeting here

It also has an update to the Phase 2 ntuple maker used in validation studies. This update saves more information about the track the hits belong to and the local angles used in the hit reconstruction. Because the ntuple maker now requires the trajectory as an input configuration files that use it must be changed, the example configuration file in the same directory has been updated to reflect this.

@mmusich @tsusa @emiglior @mtosi @tvami @pmaksim1 @jalimena @perrotta @slava77

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32220/19967

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @OzAmram (Oz Amram) for master.

It involves the following packages:

CondFormats/SiPixelTransient
Configuration/AlCa
RecoLocalTracker/SiPixelRecHits
SLHCUpgradeSimulations/Geometry

@perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @tlampen, @christopheralanwest, @ianna, @mdhildreth, @cmsbuild, @jpata, @yuanchao, @tocheng, @slava77, @ggovi, @pohsun, @kpedro88 can you please review it and eventually sign? Thanks.
@fabiocos, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @gpetruc, @dkotlins, @tocheng, @ferencek, @mtosi, @mmusich, @threus, @ebrondol, @seemasharmafnal, @tvami 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

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 20, 2020

The tests are being triggered in jenkins.

@mmusich
Copy link
Contributor

mmusich commented Nov 20, 2020

@cvuosalo I am expecting this round of tests to fail. An update of SLHCUpgradeSimulations/Geometry/test/phase2_digi_reco_pixelntuple_cfg.py is needed.

@@ -2,39 +2,40 @@
# using:
# Revision: 1.19
# Source: /local/reps/CMSSW/CMSSW/Configuration/Applications/python/ConfigBuilder.py,v
# with command line options: step2 --conditions auto:phase2_realistic -s DIGI:pdigi_valid,L1,L1TrackTrigger,DIGI2RAW,HLT:@fake2,RAW2DIGI,L1Reco,RECO --datatier GEN-SIM-RECO -n 10 --geometry Extended2026D41 --era Phase2 --eventcontent FEVTDEBUGHLT --filein file:SingleMuPt1000_pythia8_cfi_GEN_SIM.root --runUnscheduled --no_exec
# with command line options: step2 --conditions auto:phase2_realistic -s DIGI:pdigi_valid,L1,L1TrackTrigger,DIGI2RAW,HLT:@fake2,RAW2DIGI,L1Reco,RECO --datatier GEN-SIM-RECO -n 10 --geometry Extended2023D41 --era Phase2 --eventcontent FEVTDEBUGHLT --filein file:SingleMuPt1000_pythia8_cfi_GEN_SIM.root --runUnscheduled --no_usmcsexec
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to this comment look wrong. "Extended2023D41" does not exist anymore. And what is "no_usmcsexec"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I think this is left over from some very old code. I updated the comment

@cmsbuild
Copy link
Contributor

-1

Tested at: d90c0f0

CMSSW: CMSSW_11_2_X_2020-11-20-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7d7586/10913/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test testPhase2PixelNtuple had ERRORS

@cmsbuild
Copy link
Contributor

Comparison job queued.

@OzAmram
Copy link
Contributor Author

OzAmram commented Nov 20, 2020

I just pushed a new commit that updated SLHCUpgradeSimulations/Geometry/test/phase2_digi_reco_pixelntuple_cfg.py and should fix the failed test.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3375 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2529593
  • DQMHistoTests: Total failures: 33833
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2495737
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 148 log files, 34 edm output root files, 35 DQM output files

@kpedro88
Copy link
Contributor

kpedro88 commented Dec 2, 2020

Comparison differences are limited to Phase 2 workflows. Reconstructed tracks see minor changes, which propagate downstream:
all_OldVSNew_TTbar14TeV2026D49wf23234p0c_recoTracks_generalTracks__RECO_obj_chi2_2

@kpedro88
Copy link
Contributor

kpedro88 commented Dec 2, 2020

+upgrade

@makortel
Copy link
Contributor

makortel commented Dec 2, 2020

Yes I think the warning collector clearly needs to be reworked a bit and maybe it should be discussed more exactly what behavior we want.

(this is probably a wrong place to continue discussion) One option would be to utilize MessageLogger itself. If each of the "unique message" would have unique category as well, in principle MessageLogger could be configured to limit the number messages shown for those categories (e.g. to 1). MessageLogger still counts all the messages of that category, and those counts can e.g. be put into the Event and propagated to DQM. This option would need some solution for #32161 (comment).

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 2, 2020

+1

@mmusich
Copy link
Contributor

mmusich commented Dec 7, 2020

@perrotta, @christopheralanwest, @jpata, @yuanchao, @slava77, @ggovi,
could you please let us know if you have other comments on this PR and eventually sign?
Thanks

@slava77
Copy link
Contributor

slava77 commented Dec 9, 2020

+1

for #32220 f3c5e30

  • code changes are in line with the PR description and the follow up review; the most recent update now reduced the changes in reco to just the update in CondFormats/SiPixelTransient, where the GenError inputs for phase2 were updated
  • jenkins tests pass and comparisons with the baseline show (relevant) differences only in the phase-2 workflow; the differences at the level of tracks are generally small

@silviodonato
Copy link
Contributor

kind reminder @cms-sw/alca-l2 @ggovi

@yuanchao
Copy link
Contributor

yuanchao commented Dec 9, 2020

+1

  • no errors/warnings on static checks; python configurations build ok
  • use of messagelogger
  • gvz is actually 1/gvz which is not very straight forward...

@qliphy
Copy link
Contributor

qliphy commented Dec 13, 2020

kind reminder @cms-sw/db-l2

@silviodonato
Copy link
Contributor

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