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 for JPTFastjet correction chains #18121

Merged
merged 9 commits into from Apr 23, 2017

Conversation

kodolova
Copy link
Contributor

This request follows PR17900 for changes with Task
The reason of change is that JPTjets have the L1 corrections in two steps: firstly Calojets (part of JPTjet) are corrected for L1Offset or L1Fastjet and then corrections for zero suppression is done. Currently it is correctily implemented only for L1Offset. For chains that includes CaloL1Fastjet, zsp corrections are missed and this is 20% of jet energy scale.
Concerning rebase/update, I see that Configuration is completely changed. Thus, this is not a matter of rebase. I sent message to JetMET conveners and ask how to proceed.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kodolova (kodolova) for master.

It involves the following packages:

JetMETCorrections/Configuration

@perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @ahinzmann, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 29, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18792/console Started: 2017/03/29 18:02

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2017

@kodolova :
1 - I realize now that your comment "Concerning rebase/update, I see that Configuration is completely changed. Thus, this is not a matter of rebase. I sent message to JetMET conveners and ask how to proceed." was just copied from the previous #17900. Therefore, I imagine we are not supposed to wait here for any JetMET conveners' blessing of this new PR before proceeding in the scrutiny: am I correct?
2 - Could you please point me to a wf which actually includes CaloL1Fastjet in the chain? I tested a few, but I could not spot any difference in jets: quite likely I just picked out the wrong ones

@kodolova
Copy link
Contributor Author

kodolova commented Apr 4, 2017

  1. No, I just asked Jetmet POG JERC subgroup conveners whether somebody from jetmet makes the constant editing. The answer was - no and that I can go ahead with JPT chains. I can send you the copy of this e-mail.
  2. This is the historical naming problem that makes a confuse... ak4JPTL1 vs ak4L1JPT...
    F.e. ak4JPTL1FastL2L3Corrector: change: ak4JPTL1FastjetCorrector->ak4L1JPTFastjetCorrector.
    ak4JPTL1FastjetCorrector = ak4CaloL1FastjetCorrector.clone() is just a clone of the ak4CaloL1Fastjet (this is wrong for JPT and I removed this confusing line) while
    ak4L1JPTFastjetCorrector = cms.EDProducer(
    'L1JPTOffsetCorrectorProducer',
    level = cms.string('L1JPTOffset'),
    algorithm = cms.string('AK4JPT'),
    offsetService = cms.InputTag('ak4CaloL1FastjetCorrector')
    )
    If you look to JetMETCorrections/Algorithms/src/L1JPTOffsetCorrectorImpl.cc
    if (offsetService_) {
    offset = offsetService_->correction(*rawcalojet);
    }
    TLorentzVector JPTrawP4(rawcalojet->px(),rawcalojet->py(),rawcalojet->pz(),rawcalojet->energy());
    FactorizedJetCorrectorCalculator::VariableValues values;
    values.setJPTrawP4(JPTrawP4);
    values.setJPTrawOff(offset);
    values.setJetE(fJet.energy());
    result = corrector_->getCorrection(values);

This is the correct implementation of the JPT jets. It takes both L1Calo and ZSP corrections and make a combination. Chains should start with call for L1JPTOffsetCorrectorProducer

@kodolova
Copy link
Contributor Author

kodolova commented Apr 4, 2017

Henning Kirschenmann
[Reply] [Reply All] [Forward]
Actions
In response to the message from Olga Kodolova, Wed 29/03
To:
[Available] Olga Kodolova‎; [Unknown] Anastasia Karavdina
Cc:
[Offline] Seema Sharma‎; [Offline] Robert Schoefbeck
Inbox
29 March 2017 16:29
You replied on 29/03/2017 17:20.
Dear Olga,

hmm, I think the change that is in conflict with your PR is a huge one by David Dagenhart, touching "almost everything" in CMSSW:
ad81fe9

Definitely nothing JetMET-specific. Obviously, nobody from JetMET is trying to revert your changes in any way. Sorry for the extra-trouble, but I think you have to rebase your changes on the latest branch.

Thanks for maintaining/bugfixing those JPT-sequences in any case!

Cheers,
Henning

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2017

No need to send any copy of the message! I only meant that I got confused by the comment in this PR, which was referred to the old one instead, and that's why I did not start reviewing it at once. (Too late, you already sent it... ok, thank you anyhow)

About the fix, I'm not questioning its validty or correctness! I only would like to see the effect of that fix on at least one workflow. You say that there is a 20% effect on JES: how can I see (on validation plots, DQM or whatever) such a 20% modification of the jet energy before and after applying this fix to the baseline release?

@kodolova
Copy link
Contributor Author

kodolova commented Apr 4, 2017

I do not have plots. I can do if needed.
But I created 2 setups to show the diff. with and without ZSP corrections:
Current tag:
/afs/cern.ch/user/k/kodolova/public/CMSSW_TEST/L1L2L3/CMSSW_9_1_X_2017-04-04-1100/src
New tag:
/afs/cern.ch/user/k/kodolova/public/CMSSW_TEST/L1L2L3/CMSSW_9_1_X_2017-03-29-1100/src
The test python is in directories together with SQL db of one of the Spring16 version:
/afs/cern.ch/user/k/kodolova/public/CMSSW_TEST/L1L2L3/CMSSW_9_1_X_2017-04-04-1100/src/WORK
/afs/cern.ch/user/k/kodolova/public/CMSSW_TEST/L1L2L3/CMSSW_9_1_X_2017-03-29-1100/src/WORK
Begin processing the 1st record. Run 1, Event 2803, LumiSection 29
RAW JPT Jet is 74.2264 -0.825257 -3.02212 0.947183
RAW JPT Jet is 45.3292 1.30653 1.22015 1
RAW JPT Jet is 31.3694 0.436145 -1.48215 0.597057
RAW JPT Jet is 27.9357 0.324468 1.64292 1

Then with current configuration I get:
JPT123 Jet is 80.0299 -0.825257 -3.02212 0.947183
JPT123 Jet is 53.8621 1.30653 1.22015 1
JPT123 Jet is 33.2436 0.436145 -1.48215 0.597057
JPT123 Jet is 27.2698 0.324468 1.64292 1

With new tag I get:
JPT123 Jet is 90.558 -0.825257 -3.02212 0.947183
JPT123 Jet is 62.5996 1.30653 1.22015 1
JPT123 Jet is 43.7261 0.436145 -1.48215 0.597057
JPT123 Jet is 39.3439 0.324468 1.64292 1

The difference depends on pt jet, as ZSP corrections is the correction for the Calopart of jet and depends on Calojet pt.
But it is good that I check. With this rebasing I forget to change AK5->AK4 for Calo and JPT jets - done now.

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2017

Thank you, that's fine with me.
Just one question: what do you mean with "With this rebasing I forget to change AK5->AK4 for Calo and JPT jets - done now."? Should we expect an update in this PR, or were you talking about your private test setup instead?
If you confirm that this PR is final I will sign it off

@cmsbuild
Copy link
Contributor

Pull request #18121 was updated. @perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please check and sign again.

@kodolova
Copy link
Contributor Author

I changed Ak4 to AK5 for Calojets. I checked with runMatrix - no errors.

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 11, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19105/console Started: 2017/04/11 17:06

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • You potentially added 33 lines to the logs
  • Reco comparison results: 1630 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1921107
  • DQMHistoTests: Total failures: 23980
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1896954
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@perrotta
Copy link
Contributor

+1
(repeating here my previos comment #18121 (comment) )
Code looks ok
No effect on standard workflows, while the effect of the fixed scale factor was shown in #18121 (comment)

Of course, this is just to have the L1Fastjet chain is in place for JPT; as soon as the AK4 payloads for calojets will be available in the relevant GTs the authors will take care of updating the configurations touched by this PR

@franzoni
Copy link

cross-post from private email thread

Dear Olga,

we looked further into the details to find out that the error from a missing record+label (label: AK4Calo, AK4JPT ) pair emerged in the run1 8 TeV workflow. Such labels are present in the 2017 data and MC workflows.

Why do you need to modify the jet correction sequence of run1 ? I would imagine you woulds handle such sequence by era, and focus on the 13 TeV scenarios.
That question aside, if you want to have the record+label (label: AK4Calo, AK4JPT ) pair included in the run1 GT for 91x, the steps are :

. do the relevant tags measured at 8 TeV exist ? The tags with the same label present in the 13 TeV GT's are not necessarily consistent with usage at 8 TeV

. if they exist, the tags need to be presented, queued and announced in hn, see the procedure documented here https://twiki.cern.ch/twiki/bin/view/CMS/UserTagsInTheGTProcedure#Current_procedure_during_Run_II

HTH, Cheers,
Arun and Giovanni

@kodolova
Copy link
Contributor Author

Answer to private mail thread

Dear Giovanni,

  1. I do not want to change reco sequence for 8 TeV and I do not want to change sequence for run 1.

  2. The algorithm radius can not be handle with era.

    For Run I we had AK5 and AK7 for Calo and JPT. For run 2 we have AK4 and AK7 for Calo/JPT. But this does not mean that we will be ask to have AK5 tables for Calo if some
    analyses would like to make the fair comparison Run 2 to Run I without shape rescaling or for some other studies.

    Thus, situation that ak5 reconstruction chain is connected with AK5 database rag is unnatural and will lead to a mess in future.

  3. relating other questions:

    For Run 1 there was not AK4 and there was not corresponding tag for Calo jets.

Ideally for 8 TeV it should be ak5 reconstruction chain in Validation tests and for 13 TeV ak4 reconstruction chain.
Best regards,
Olga
P.S. Perhaps, one could copy AK5 tables to AK4 for Run I? It will give the same effect as use AK5 tables with ak4 reconstruction.

@slava77
Copy link
Contributor

slava77 commented Apr 13, 2017 via email

@kodolova
Copy link
Contributor Author

We do not need to support AK5 for 91X for JPT. The problem is not with JPT. The problem is with Calojets and the historical artefact of jet energy corrections configuration.

@davidlange6
Copy link
Contributor

hi @slava77 @perrotta @kodolova ,all - are there unanswered issues from the discussion above? I had assumed the discussion would continue, but it seems not.

@perrotta
Copy link
Contributor

@davidlange6 : the last discussion was on the part of this PR which got reverted (i.e. ak4 for calojets) , and therefore kept out of this PR. It must continue, but probably it is better if it is out of this thread.

For what this PR is concerned, my comment in #18121 (comment) still stands: "Of course, this is just to have the L1Fastjet chain is in place for JPT; as soon as the AK4 payloads for calojets will be available in the relevant GTs the authors will take care of updating the configurations touched by this PR"

@kodolova
Copy link
Contributor Author

I discussed the new GT with JERC subgroup of JetMET. They promised to have it within 2 weeks but they promised to have new corrections already a month ago. These 2 weeks may come to month or so. May I propose to integrate this PR as it is if all tests as ok.
As soon as JetMET finished with JEC and will prepare GT, I will make the needed change and will open it as the new PR?

@davidlange6
Copy link
Contributor

Hi @perrotta - i'm not sure if I can determine a "yes" or "no" from your answer. if the PR is not ready to go, could you put it on hold, otherwise I'll assume your +1 still stands

@perrotta
Copy link
Contributor

@davidlange6 : my answer meant "yes, I confirm my +1 (and I assume that further updates will come in a future new PR)"

@davidlange6 davidlange6 merged commit 93dc1eb into cms-sw:master Apr 23, 2017
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

7 participants