Switch default HBHENoiseFilter settings to Run2-25ns configuration V2 - 76X

[dertexaner]

This is take-2 to switch the default HBHENoiseFilterResultProducer settings to Run2-25ns configuration such that users get correct noise flags out of the box for the bulk of 2015 data (and beyond) which is with 25ns bunch spacing.
As per @slava77's request (see PR #11778 ), Eras is replaced by BunchSpacingProducer in HBHENoiseFilterResultProducer.
In the default settings, BunchSpacingProducer is enabled, which should automatically switch between Run2 50ns and 25ns settings. However, users will have to manually configure and rerun HBHENoiseFilterResultProducer in case of any Run1 50ns data analysis.
@abdoulline @igv4321 @mariadalfonso

[cmsbuild]

A new Pull Request was created by @dertexaner for CMSSW_7_6_X.

Switch default HBHENoiseFilter settings to Run2-25ns configuration V2 - 76X

It involves the following packages:

CommonTools/RecoAlgos

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @makortel, @abbiendi, @jhgoh, @ahinzmann this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[slava77]

I think this (IgnoreTS4TS5ifJetInLowBVRegion) is still useful and should stay.
My request was towards removing configuration-level settings to switch between 50ns or 25 ns in the same major period of data taking as we had in 2015

run1 vs run2 configuration changes should be OK for some time (trying to keep this down to detector level changes).

[slava77]

I'm still expecting the
eras.run2_common.toModify(HBHENoiseFilterResultProducer, IgnoreTS4TS5ifJetInLowBVRegion=False)
to return back to the file and "True" to be in the run1 config.

[dertexaner]

Ready.

[dertexaner]

My impression was that the dependence on Eras (I guess, more specifically on run2_25ns_specific vs run2_50ns_specific) was proving to be problematic and resulted in effectively everyone rerunning the HBHENoiseFilterResultProducer at the analysis level. The current PR would eliminate this, with the very limited exception of Run1 analyses.
Is there any risk for "eras.run2_common" not to be specified properly? If so, I'd prefer keeping Eras out of the picture altogether since it only caused confusion so far.

[slava77]

On 10/14/15 5:08 PM, dertexaner wrote:

My impression was that the dependence on Eras (I guess, more
specifically on run2_25ns_specific vs run2_50ns_specific) was proving to
be problematic and resulted in effectively everyone rerunning the
HBHENoiseFilterResultProducer at the analysis level. The current PR
would eliminate this, with the very limited exception of Run1 analyses.
Is there any risk for "eras.run2_common" not to be specified properly?
If so, I'd prefer keeping Eras out of the picture altogether since it
only caused confusion so far.

The problematic part is having to configure 25ns and 50ns separately
during run2,

• some datasets can have both kinds of fills data included
• people processing both 25ns and 50ns datasets (MC and data) will have less
  changes to take care of and will not have to remember to make the switch
  if we manage to eliminate this configuration level dependence of 25ns
  vs 50ns.

—
Reply to this email directly or view it on GitHub
#11789 (comment).

[dertexaner]

I think I understand . But just to clarify, the configuration in this PR requires no action on the part of users neither in 50ns Run2 nor 25ns Run2 scenarios, and it doesn't depend on any Eras customization.
IgnoreTS4TS5ifJetInLowBVRegion is really a Run1 vs Run2 difference, and not a 50ns vs 25ns difference.
If your point is that Run1 vs Run2 changes should also be automated, then yes, we will need to rely on Eras. Is this the case?

[slava77]

On 10/14/15 6:04 PM, dertexaner wrote:

I think I understand . But just to clarify, the current configuration
requires no change in 50ns Run2 and 25ns Run2 scenarios, and it doesn't
depend on any Eras customization.

Your new code changes indeed make it automatic at runtime to figure out
if it's 50ns or 25 ns.
Meaning that the same cmsRun config.py will work if input file has 50ns
or 25 ns data.

Previously, even with eras and 25ns specific settings you would have to
have different config.py files
to correctly process inputs with 25ns, to be running separately on a
different config py file for inputs with 50 ns. (I thought it was kind
of obvious)

If your point is that Run1 vs Run2 changes should also be automated,
then yes, we will need to rely on Eras. Is this the case?

considering use cases (rarely run1 and run2 data mixed together in one
analysis and the same
release), using different configurations (via eras, preferably, now) is OK.

—
Reply to this email directly or view it on GitHub
#11789 (comment).

[slava77]

also, please cleanup customise_DQM_25ns function so that it no longer has the modification of HBHENoiseFilterResultProducer
https://github.com/cms-sw/cmssw/blob/CMSSW_7_6_0_pre7/SLHCUpgradeSimulations/Configuration/python/postLS1Customs.py#L193

Thank you.

[slava77]

@dertexaner
please clarify if you can update the customise_DQM_25ns mentioned in #11789 (comment)
With current changes in this PR it already becomes redundant (changes in the PSet there will do nothing)

[dertexaner]

Hopefully that worked.

[cmsbuild]

Pull request #11789 was updated. @cmsbuild, @cvuosalo, @civanch, @mdhildreth, @slava77 can you please check and sign again.

[slava77]

@cmsbuild please test

@dertexaner thank you

[cmsbuild]

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

[cmsbuild]

+1
Tested at: 9acf9a8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11789/8952/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
9acf9a8
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11789/8952/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11789/8952/git-merge-result

[cmsbuild]

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

[civanch]

+1

[slava77]

+1

for #11789 9acf9a8

• technical change to automate choice of default outputs depending on the bunch spacing
• jenkins tests pass and comparisons with baseline shows no differences
• additionally tested locally in CMSSW_7_6_X_2015-10-19-1100, including run2 134.{7,8}0* workflows: no changes were observed and comparison of expanded run1 and run2 reco+pat workflow steps shows expected changes

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

[davidlange6]

+1