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

Add CSCCluster skim config #37782

Merged
merged 5 commits into from Jul 21, 2022
Merged

Add CSCCluster skim config #37782

merged 5 commits into from Jul 21, 2022

Conversation

kakwok
Copy link
Contributor

@kakwok kakwok commented May 3, 2022

PR description:

This PR adds the config for a RAW-AOD skim for the CSC clusters events, based on new LLP trigger paths.
The CSC cluster skim saves the following collections in addition to standard RAW-AOD output:

csc2DRecHits
dt1DRecHits

The CSC cluster skim selects events passing the following HLT paths, which is already included in the HLT v2 menu:

HLT_CscCluster_Loose_v*
HLT_CscCluster_Medium_v*
HLT_CscCluster_Tight_v*
HLT_L1CSCShower_DTCluster50_v2
HLT_L1CSCShower_DTCluster75_v2

HLT Jira ticket

PR validation:

Tested with a high-Pt QCD sample in CMSSW_12_4_0_pre3

/RelValQCD_Pt_1800_2400_14/CMSSW_12_3_0_pre6-123X_mcRun3_2021_realistic_v11-v2/GEN-SIM-RECO

The skimmed file size for was 12MB, and the size of the unskimmed AOD file was 1.9GB with 1951 events.

The test script (test_CSCCluster_cfg.py), generated with cmsDriver, is attached in the PR.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37782/29688

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2022

A new Pull Request was created by @kakwok for master.

It involves the following packages:

  • Configuration/Skimming (pdmv)

@cmsbuild, @bbilin, @kskovpen, @jordan-martins can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @fabiocos 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

@sam7k9621
Copy link
Contributor

sam7k9621 commented May 9, 2022

Hi @kakwok
I have tested with the provided test script using the corresponding four root files:

'/store/relval/CMSSW_12_3_0_pre6/RelValQCD_Pt_1800_2400_14/GEN-SIM-RECO/123X_mcRun3_2021_realistic_v11-v2/10000/649f3446-1698-4910-b879-9a6d94d62a9b.root',
'/store/relval/CMSSW_12_3_0_pre6/RelValQCD_Pt_1800_2400_14/GEN-SIM-RECO/123X_mcRun3_2021_realistic_v11-v2/10000/aa515779-dad9-4994-b7d2-d672a7c8938a.root',
'/store/relval/CMSSW_12_3_0_pre6/RelValQCD_Pt_1800_2400_14/GEN-SIM-RECO/123X_mcRun3_2021_realistic_v11-v2/10000/b7315dce-732e-4ec7-b137-ee3bafff1cd6.root',
'/store/relval/CMSSW_12_3_0_pre6/RelValQCD_Pt_1800_2400_14/GEN-SIM-RECO/123X_mcRun3_2021_realistic_v11-v2/10000/df8bd2c1-1e45-479d-9287-2d40fecc25d8.root'

However, the resulting unskimmed file is 8.7GB and the skimmed file is 39 MB.
Could you please let me know which dataset you were testing with?

@kakwok
Copy link
Contributor Author

kakwok commented May 9, 2022

Hi @sam7k9621 ,

Yes, those are the 4 root files I used, but I didn't run over all the events in the file.

@smuzaffar smuzaffar modified the milestones: CMSSW_12_4_X, CMSSW_12_5_X May 16, 2022
@kakwok
Copy link
Contributor Author

kakwok commented Jun 20, 2022

Hi @sam7k9621 Can this be merged now?

@perrotta
Copy link
Contributor

please test

paths = (EXOCSCClusterPath),
content = skimRawAODContent.outputCommands+['keep *_csc2DRecHits_*_*','keep *_dt1DRecHits_*_*'],
selectEvents = cms.untracked.PSet(),
dataTier = cms.untracked.string('AOD')
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't seem that "AOD" is the correct data tier for this event content...

Also by changing skimRawAODContent here, you change it for everyone. If that is intended, the additional keep statements should go instead into Configuration.Skimming.PDWG_cff.py

Copy link
Contributor Author

@kakwok kakwok Jun 20, 2022

Choose a reason for hiding this comment

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

I think I'm just taking the value of skimRawAODContent output command and adding the extra collections that I need here, which shouldn't change what's in skimRawAODContent for everyone?
What should I put for the data tier here then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. - yes, you are correct I agree. I'm not sure what naming convention is being followed for raw+aod skims. (Hopefully it is not AOD)?

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 followed the convention in this PR (similar skim for DTClusters):
https://github.com/cms-sw/cmssw/pull/37509/files

Copy link
Contributor

Choose a reason for hiding this comment

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

@kskovpen so it this really supposed to be AOD after all?

Copy link
Contributor

@sam7k9621 sam7k9621 Jul 20, 2022

Choose a reason for hiding this comment

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

I think it can be modified as "USER" to follow EXODisplacedJet

Copy link
Contributor

Choose a reason for hiding this comment

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

(hmm it's not supposed to be my business, but since I'm following the PR...) isnt USER only meant for private productions from the users? I saw the other skims mostly use RAW-RECO, why shouldnt this use that too?

Copy link
Contributor

@sam7k9621 sam7k9621 Jul 20, 2022

Choose a reason for hiding this comment

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

In my understanding, different “types” only affect how it will be categorized in the DAS.
Since it shares the same content with EXODisplacedJet, I think we can follow the settings

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be USER, it's AOD with customized event content.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kakwok please modify it to USER then

)

# Additional output definition
process.SKIMStreamEXOCSCCluster = cms.OutputModule("PoolOutputModule",
Copy link
Contributor

Choose a reason for hiding this comment

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

implemented this way, the test does not test the skim as defined in the cffs above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi David, what's the right way to do so then?
the script is generated with the cmsDriver command as stated in the config:

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, cmsDriver doesn't quite do this for you - eg, you can instead import the output module configuration from your cff and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @davidlange6 It's fixed now
3515db5

@tvami
Copy link
Contributor

tvami commented Jul 21, 2022

unhold

  • change was done

@tvami
Copy link
Contributor

tvami commented Jul 21, 2022

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Jul 21, 2022

@kakwok can you please also prepare the 12_4_X backport of this PR? thanks!

@tvami
Copy link
Contributor

tvami commented Jul 21, 2022

Not for this PR, but

The test script (test_CSCCluster_cfg.py), generated with cmsDriver, is attached in the PR.

This should be made a unit test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37782/31172

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #37782 was updated. @bbilin, @kskovpen, @jordan-martins can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ecd84a/26361/summary.html
COMMIT: c0a4dba
CMSSW: CMSSW_12_5_X_2022-07-20-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37782/26361/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-ecd84a/13234.0_TTbar_14TeV+2021FS+TTbar_14TeV_TuneCP5_FastSimRun3+HARVESTFastRun3
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-ecd84a/13434.0_TTbar_14TeV+2021FSPU+TTbar_14TeV_TuneCP5_FastSimRun3PU+HARVESTFastRun3PU

Summary:

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

@kskovpen
Copy link
Contributor

+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)

Comment on lines +4 to +10
CSCClusterTrigger = HLTrigger.HLTfilters.hltHighLevel_cfi.hltHighLevel.clone()
CSCClusterTrigger.TriggerResultsTag = cms.InputTag( "TriggerResults", "", "HLT" )
CSCClusterTrigger.HLTPaths = cms.vstring(
["*CscCluster*","*L1CSCShower*"]
)
CSCClusterTrigger.throw = False
CSCClusterTrigger.andOr = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but for a possible cleaning in another PR:

Suggested change
CSCClusterTrigger = HLTrigger.HLTfilters.hltHighLevel_cfi.hltHighLevel.clone()
CSCClusterTrigger.TriggerResultsTag = cms.InputTag( "TriggerResults", "", "HLT" )
CSCClusterTrigger.HLTPaths = cms.vstring(
["*CscCluster*","*L1CSCShower*"]
)
CSCClusterTrigger.throw = False
CSCClusterTrigger.andOr = True
CSCClusterTrigger = HLTrigger.HLTfilters.hltHighLevel_cfi.hltHighLevel.clone(
TriggerResultsTag = ( "TriggerResults", "", "HLT" ),
HLTPaths = ["*CscCluster*","*L1CSCShower*"],
throw = False,
andOr = True
)

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3366348 into cms-sw:master Jul 21, 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

9 participants