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 full reco workflow #14865

Merged
merged 23 commits into from Jun 24, 2016
Merged

Conversation

kpedro88
Copy link
Contributor

This PR updates the Phase2 GReco workflow to do the full RECO step (no longer just tracking reco). List of changes:

  1. several commits from @ebrondol to fix the SeedClusterRemover for Phase2 (needed for electron reco)
  2. 2023GReco and 2023tilted geometries use Run2 calorimeters (HGCal removed)
  3. Phase2 Eras refactored into Phase2sim, Phase2LReco, Phase2GReco
  4. phase2_common Era refactored into phase2_common (now only for removing preshower) and phase2_hcal
  5. Added LReco PU workflow (for PU runs with HGCal)

Tested by running upgrade workflows 11200 and 10600.

Comments welcome...

attn: @ebrondol, @boudoul, @VinInn, @ianna, @bsunanda, @lgray, @calabria

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X.

It involves the following packages:

CalibCalorimetry/HcalPlugins
Configuration/Geometry
Configuration/PyReleaseValidation
Configuration/StandardSequences
DataFormats/TrackerRecHit2D
Geometry/CMSCommonData
RecoEcal/EgammaClusterProducers
RecoJets/JetProducers
RecoLocalCalo/Configuration
RecoLocalTracker/SubCollectionProducers
RecoParticleFlow/PFClusterProducer
RecoTracker/Configuration
RecoTracker/IterativeTracking
SLHCUpgradeSimulations/Configuration
SimCalorimetry/Configuration
SimCalorimetry/EcalTrigPrimProducers
SimCalorimetry/HcalZeroSuppressionProducers
SimG4Core/Configuration
SimGeneral/MixingModule

@ghellwig, @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @fabozzi, @cmsbuild, @rekovic, @srimanob, @cerminar, @franzoni, @slava77, @mmusich, @hengne, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @yslai, @forthommel, @yduhm, @rappoccio, @argiro, @Martin-Grunewald, @threus, @mmarionncern, @makortel, @lgray, @jdolen, @jlagram, @OlivierBondu, @Sam-Harper, @wmtan, @GiacomoSguazzoni, @rafaellopesdesa, @tocheng, @VinInn, @nhanvtran, @rovere, @schoef, @mschrode, @dgulhan, @nickmccoll, @gbenelli, @ahinzmann, @cbernet, @gpetruc, @mariadalfonso, @bachtis this is something you requested to watch as well.
@slava77, @degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@kpedro88
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 13, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13474/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

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

  • 134.911_RunSinglePh2015D+RunSinglePh2015D+HLTDR2_25ns+RECODR2_25nsreHLT+HARVESTDR2

if k2 in PUDataSets:
upgradeStepDict['RecoFullGlobalPU'][k]=merge([PUDataSets[k2],upgradeStepDict['RecoFullGlobal'][k]])

upgradeStepDict['RecoFullGlobal_trackingOnlyVal'][k] = {'-s':'RAW2DIGI,L1Reco,RECO,VALIDATION:@trackingOnlyValidation,DQM:@trackingOnlyDQM',
Copy link
Contributor

Choose a reason for hiding this comment

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

@kpedro88 An alternative would be to use the standard VALIDATION and DQM sequences, and configure them to contain only the tracking validation and DQM (respectively) with eras (unless this is not just temporary). That was the approach I used for phase1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makortel - I'm just following what was used in #14827. Personally, I prefer to keep some of the very temporary stuff separate from Eras...

Copy link
Contributor

Choose a reason for hiding this comment

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

@kpedro88 Sure, if it is indeed very temporary. But I'd guess that at some point phase2 will need VALIDATION+DQM beyond tracking-only, and the modules in the sequences probably need to be enabled in steps (as happened with phase1, and in fact there are still modules disabled).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should address this in a subsequent PR. I am still discussing with other experts which subdetectors might be ready for Phase2 validation/DQM.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what would you name this once more than trackingOnly is added to the validation (e.g. trackingAndJetAndBlahAndBlah and so on)?
It may be better to have a common name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what I'll name it. Maybe phase2Val? This section of relval_steps.py is commented as "UPGRADE WORKFLOWS IN PREPARATION" for a reason. It is not always clear what the path forward will be for the upgrade development. Some allowances need to be made.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it to be an easier allowance to keep one name and not generate more and more specifically named implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are several minor annoyances that come with this further extended naming:

  • the step key is used in the directory name generated by runTheMatrix. So, it's getting longer and longer and begins to wrap around in my ls
  • I use full directory names to map inputs for jenkins comparisons. If the directory name changes, we loose a possibility to compare content.

@ianna
Copy link
Contributor

ianna commented Jun 14, 2016

+1

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jun 23, 2016

+1

for #14865 c14db63

  • since the last reco approval a LogWarning in EcalClusterLazyTools was downgraded to a LogInfo. Compilation is enough at this point.

@kpedro88
Copy link
Contributor Author

@davidlange6 what is the timeline for pre8? This still needs signatures from @civanch, @ianna, @mmusich, @hengne, @dmitrijus, @mulhearn. Hopefully there will not be any further commits needed.

@cmsbuild
Copy link
Contributor

@ianna
Copy link
Contributor

ianna commented Jun 23, 2016

+1

@franzoni
Copy link

+1

@fabozzi
Copy link
Contributor

fabozzi commented Jun 23, 2016

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment