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

Fix r45Fraction_ calculation #10724

Merged

Conversation

dertexaner
Copy link
Contributor

This is a simple bug-fix for the noisy hit fraction (r45Fraction_ ) calculation in CommonHcalNoiseRBXData.
It is highly desirable to have this in the earliest possible 74X and 75X releases.

@cmsbuild
Copy link
Contributor

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

Fix r45Fraction_ calculation

It involves the following packages:

RecoMET/METAlgorithms

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @ahinzmann, @mmarionncern, @jdolen, @nhanvtran, @schoef, @mariadalfonso 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
Copy link
Contributor

slava77 commented Aug 12, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Aug 12, 2015

let's see the comparison results.
If changes are noticeable in many events, this will go only together with the re-miniAOD release in preparation (after the 25 ns ramp up data is collected).
@gpetruc

Is this fix essential for simulation or is it affecting primarily values relevant for data?

@dertexaner
Copy link
Contributor Author

@slava77 The fix is primarily for 25ns data taking.
In the relevant rechitR45 filter, the event rejection decision is based on the "noisy hit count fraction" as well as "noisy hit energy fraction" per RBX.
This rechitR45 filter is configured to reject an event if either of these fractions are >20% (Tight WP) (or >50% (Loose WP)). Since these two ratios are very correlated, this hasn't caused a great performance degradation thus far.

@slava77
Copy link
Contributor

slava77 commented Aug 12, 2015

@dertexaner do you have any plots for before and after?
[in production releases consistency in behavior often outweighs importance of a bugfix changing physics output performance]

@dertexaner
Copy link
Contributor Author

@slava77 Not directly. Attached are two files showing the 2D distribution of noisy energy fraction and the noisy hit fraction per RBX. RBXs in gray shaded regions are declared as noisy.
Each file has two plots (left plot is with 50ns 2012 data, and right plot is with 50ns 2012 data but with OOT PU mixing to simulate 25ns conditions). The bug effectively disables the cut along the x-axis, but since we have only a few events there, we don't expect a great change in filter performance.

current
postfix

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

@cmsbuild please test
Comparisons stuck in Queued state. Maybe a second try will allow them to complete.

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1
Tested at: a4e08f8
When I ran the RelVals I found an error in the following worklfows:
50202.0 step3

runTheMatrix-results/50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50/step3_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50.log
----- Begin Fatal Exception 13-Aug-2015 21:38:02 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Processing run: 1 lumi: 1 event: 1
   [1] Running path 'validation_step'
   [2] Calling event method for module MixingModule/'mix'
Exception Message:
RootInputFileSequence::readOneSpecified(): no input files specified for secondary input source.
----- End Fatal Exception -------------------------------------------------

4.53 step1

DAS Error

1001.0 step1

DAS Error

1003.0 step1

DAS Error

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10724/7246/summary.html

@dertexaner
Copy link
Contributor Author

@slava77 Would you know what the holdup is?

@slava77
Copy link
Contributor

slava77 commented Aug 13, 2015

we'll look at it today or tomorrow.

@cvuosalo
Copy link
Contributor

@cmsbuild please test
Might DAS be working now?

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1
Tested at: a4e08f8
When I ran the RelVals I found an error in the following worklfows:
50202.0 step2

runTheMatrix-results/50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50/step2_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50.log
----- Begin Fatal Exception 14-Aug-2015 04:35:48 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Processing run: 1 lumi: 1 event: 1
   [1] Running path 'digitisation_step'
   [2] Calling event method for module MixingModule/'mix'
Exception Message:
RootInputFileSequence::readOneRandom(): no input files specified for secondary input source.
----- End Fatal Exception -------------------------------------------------

25202.0 step3

runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log
----- Begin Fatal Exception 14-Aug-2015 04:46:14 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Processing run: 1 lumi: 1 event: 1
   [1] Running path 'validation_step'
   [2] Calling event method for module MixingModule/'mix'
Exception Message:
RootInputFileSequence::readOneSpecified(): no input files specified for secondary input source.
----- End Fatal Exception -------------------------------------------------

4.53 step1

DAS Error

140.53 step1

DAS Error

1003.0 step1

DAS Error

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10724/7254/summary.html

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #10724 a4e08f8

Correcting a coding mistake to properly calculate the RBX HCAL noisy hits fraction. #10725 and #10726, the 75X and 76X versions of this PR, have already been merged.

The code change is satisfactory, and Jenkins tests against baseline CMSSW_7_4_X_2015-08-14-1100 show no significant differences, since the fix covers a case that rarely occurs in standard workflows. A test of prompt reco against baseline CMSSW_7_4_9 using file /store/data/Run2015C/MET/RAW/v1/000/254/231/00000/B6DF41DA-8E41-E511-9DA0-02163E01476B.root shows no significant differences. Testing seems to indicate that the effect of this bug fix is very limited.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs once checked with relvals in the development release cycle of CMSSW (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Aug 19, 2015
@cmsbuild cmsbuild merged commit fb7bf01 into cms-sw:CMSSW_7_4_X Aug 19, 2015
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.

5 participants