Making Puppi optional for PATPackedCandidate producer : 90X #16777
Conversation
|
A new Pull Request was created by @Sam-Harper for CMSSW_9_0_X. It involves the following packages: PhysicsTools/PatAlgos @cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
| minPtForTrackProperties_(iConfig.getParameter<double>("minPtForTrackProperties")) | ||
| minPtForTrackProperties_(iConfig.getParameter<double>("minPtForTrackProperties")), | ||
| usePuppi_(!iConfig.getParameter<edm::InputTag>("PuppiSrc").encode().empty() || | ||
| !iConfig.getParameter<edm::InputTag>("PuppiNoLepSrc").encode().empty()) |
There was a problem hiding this comment.
I would move the calls to consumes<> for the Puppi stuff inside the body of the method, inside a if(usePuppi_) {...} (or bring the usePuppi_ higher up in the list of arguments, and do e.g. PuppiWeight_(usePuppi_ ? consumes<...>() : edm::EDGetTokenT<...>()))
There was a problem hiding this comment.
so to both comments. I can do this but I'm merely fixing the half implemented functionality somebody else put in. So what I've done I consider "safe" and it is a bug fix which was intentionally extremely minimal. Do you really need the rest of the changes?
| @@ -129,7 +135,7 @@ void pat::PATPackedCandidateProducer::produce(edm::StreamID, edm::Event& iEvent, | |||
| iEvent.getByToken( PuppiCandsMap_, puppiCandsMap ); | |||
| edm::Handle<std::vector< reco::PFCandidate > > puppiCands; | |||
| iEvent.getByToken( PuppiCands_, puppiCands ); | |||
| std::vector<int> mappingPuppi(puppiCands->size()); | |||
| std::vector<int> mappingPuppi(usePuppi_ ? puppiCands->size() : 0); | |||
There was a problem hiding this comment.
can we put all the getByToken of the puppi's and the loop of line 147-149 in an if (usePuppi_)? (at that point, the isValid will no longer be needed)
also, I would replace the other isValid() calls to usePuppi_ (e.g. line 266)
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons |
|
@Sam-Harper: What is the plan for resolving the issues raised by @gpetruc? I am holding off review until the code in this PR is finalized. |
|
@Sam-Harper: If you want this PR to go into 90pre2, the issues need to be resolved now. Please don't wait until the end of the week for revisions. |
|
sadly I was away at a workshop. This is why I didnt to do any changes unless there is a good reason, @gpetruc never responded to my comment on this. @gpetruc, is this change something that is "nice in a theoretical sense in an ideal world where everybody has infinite time" or is this change something that is important. Will not doing it have a negative performance impact? |
|
So I have made the changes and tested them. Once the integration tests (runTheMatrix.py -l limited -i all) have completed, I will update the branches. |
cmsbuild
added
comparison-pending
and removed
comparison-available
tests-approved
labels
|
Comparison job queued. |
|
Comparison is ready The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons |
|
+1 Provide the option to run the PAT PackedCandidate producer without PUPPI. Default behavior does not change. There should be no change in monitored quantities. #16776 and #16778 are the 80X and 81X versions of this PR. The code changes are satisfactory, and Jenkins tests against baseline CMSSW_9_0_X_2016-12-07-0000 show no significant differences, as expected. |
90X version of #16776
This commit makes puppi inputs optional for PATPackedCandidateProducer. If the puppi related input tags are both empty, it disables accessing puppi. If the input tags not empty, the behaviour is identical to before, thereby protecting against simple typos.
This is helpful to remake the PFPackedCandidates without running all of miniAOD. The use case which prompts this is the new HEEP V7.0 uses PFPackedCandidates for tracker isolation and therefore needs to produce these in AOD.