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

Cleanup duplicate pileup jet ID configurations #10625

Merged
merged 1 commit into from Aug 20, 2015

Conversation

ahinzmann
Copy link
Contributor

A cleanup of the PU jet ID configs which are duplicate as a followup of #10398.

The configs under 1. are kept while those under 2. and 3. are dropped and their content is integrated into 1.:

This one is from 2013 and kept in sync with MVA MET in 2013:
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/PileupJetID_cfi.py
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/PileupJetIDParams_cfi.py
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/PileupJetIDCutParams_cfi.py
It is used in MET.

This one was introduced in 2014 and kept in sync with MET in 2014:
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/pileupjetidproducer_cfi.py
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/puJetIDParams_cfi.py
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/RecoJets/JetProducers/python/puJetIDAlgo_cff.py
It is used in MET and DQM.

This one was introduced in 2014 for MiniAOD and is yet another copy:
https://github.com/jbrands/cmssw/blob/newOfflinePuID_new/PhysicsTools/PatAlgos/python/slimming/pileupJetId_cfi.py

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2015

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

Cleanup duplicate pileup jet ID configurations

It involves the following packages:

DQM/PhysicsHWW
DQMOffline/JetMET
PhysicsTools/PatAlgos
RecoJets/JetProducers
RecoMET/METPUSubtraction

@cvuosalo, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @danduggan can you please review it and eventually sign? Thanks.
@rappoccio, @imarches, @yslai, @acaudron, @mmarionncern, @jdolen, @nhanvtran, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder, @TaiSakuma, @rociovilar 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 7, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2015

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 7, 2015

@cvuosalo
Copy link
Contributor

cvuosalo commented Aug 9, 2015

+1

For #10625 a83e421

Clean-up of duplicate pile-up jet ID configurations. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-08-07-1100 show no significant differences, as expected. A check of the expanded RECO config for workflow 25202.0 confirms that the code changes do appear in the config.

@gpetruc
Copy link
Contributor

gpetruc commented Aug 11, 2015

When this PR is ready, can you prepare the backports for this + #10398?
Thanks,
Giovanni

@ahinzmann
Copy link
Contributor Author

FYI: There is currently a bug in this new offline jet ID code. Only once this is fixed and validated, we can make the backport.

@ahinzmann
Copy link
Contributor Author

Bug fixed in #10730. Validation ongoing.

@deguio
Copy link
Contributor

deguio commented Aug 14, 2015

+1

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2015

@monttj please check this one
Thank you

@monttj
Copy link
Contributor

monttj commented Aug 19, 2015

+1

@cmsbuild
Copy link
Contributor

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 added a commit that referenced this pull request Aug 20, 2015
Cleanup duplicate pileup jet ID configurations
@davidlange6 davidlange6 merged commit 77cdd39 into cms-sw:CMSSW_7_6_X Aug 20, 2015
@gpetruc
Copy link
Contributor

gpetruc commented Aug 21, 2015

backports?

@ahinzmann
Copy link
Contributor Author

While software-wise everything is validated, the training weights are still being validated and until this is done, backport to 74/75 is on hold.
Still need to check

  1. this training is optimal for ak4 (for mvaMET) and ak4CHS (for jets) my comparing two separate trainings.
  2. this training performs well for mvaMET.

@gpetruc
Copy link
Contributor

gpetruc commented Aug 24, 2015

Is what we currently have in 74X good for physics?
If that one isn't good anyway, then I'd rather have this one if it has a
chance of being good: at worst, if the validation fails, we will have just
replaced one random number with another random number, but if instead the
validation is successful people will be able to use it.
This is especially true for the more basic scenario of people using it on
the ak4CHS jets: mvaMET anyway is currently not in MiniAOD and so the few
people who use need to run more complex recipes and they can recompute a
new PuJetID if needed, while the same computation would add some burden to
the much larger number of users of the slimmedJets.

On Mon, Aug 24, 2015 at 2:58 PM, ahinzmann notifications@github.com wrote:

While software-wise everything is validated, the training weights are
still being validated and until this is done, backport to 74/75 is on hold.
Still need to check

  1. this training is optimal for ak4 (for mvaMET) and ak4CHS (for jets) my
    comparing two separate trainings.
  2. this training performs well for mvaMET.


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

@ahinzmann
Copy link
Contributor Author

ok, if mvaMET is not in MiniAOD, including this one would make sense.
@jbrands : Since you started validating this one yesterday, could you post the new ROC curve comparison of 53 vs 74 here? (based on the current weights, not the new ones which are still in the works)

@ahinzmann
Copy link
Contributor Author

Johannes confirmed that this discriminant performs better than the 53X training in 76X (though not reaching the Run I performance yet).

roc_pt20to30_eta0to2
roc_pt20to30_eta2to3
roc_pt20to30_eta3to5
roc_pt30to50_eta0to2
roc_pt30to50_eta2to3
roc_pt30to50_eta3to5

Starting backport to 74 and 75.

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

8 participants