Initial implementation of Plan 1 rechit combination #17313
Conversation
|
A new Pull Request was created by @igv4321 (Igor Volobouev) for CMSSW_9_0_X. It involves the following packages: RecoLocalCalo/HcalRecAlgos @cmsbuild, @cvuosalo, @slava77, @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. |
|
-1 Tested at: e7397c7 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: AddOn
I found errors in the following addon tests: cmsDriver.py RelVal -s L1REPACK:GT1 --data --scenario=pp -n 10 --conditions auto:run1_hlt_Fake --relval 9000,50 --datatier "RAW" --eventcontent RAW --customise=HLTrigger/Configuration/CustomConfigs.L1T --fileout file:RelVal_Raw_Fake_DATA.root --filein /store/data/Run2012A/MuEG/RAW/v1/000/191/718/14932935-E289-E111-830C-5404A6388697.root : FAILED - time: date Sun Jan 29 22:44:22 2017-date Sun Jan 29 22:36:19 2017 s - exit: 23552 |
|
Comparison job queued. |
|
It looks like some input files are missing. The test failure is, of course, unrelated to this PR. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
|
| @@ -0,0 +1,21 @@ | |||
| import FWCore.ParameterSet.Config as cms | |||
|
|
|||
| hbheprereco = cms.EDProducer( | |||
There was a problem hiding this comment.
Can you define a unique name for this module?
| "HBHEPlan1Combiner", | ||
|
|
||
| # Label for the input HBHERecHitCollection | ||
| hbheInput = cms.InputTag("FIX THIS!!!"), |
There was a problem hiding this comment.
the tag here should be "hbheprereco"
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-rejected
labels
|
Pull request #17313 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
@kpedro88 |
|
@cmsbuild please test |
|
OK, comments removed. Hopefully, this increases the overall
level of happiness.
…On 02/06/2017 09:56 AM, Slava Krutelyov wrote:
On 2/6/17 7:41 AM, Igor Volobouev wrote:
> Do I really have to worry about this? Obviously,
> the authors of "mkedprod" wanted to keep these
> comments in. It makes sense to me -- in the future,
> if I want to implement one of these functions,
> I don't have to look up the interface definitions.
>
> Now, imagine applying your comments to all
> CMSSW module authors. Does it make sense?
> Not really -- instead, one should modify "mkedprod".
The skeleton should contain all relevant methods.
The actual code doesn't really have to,
if it's not using them.
>
> On 02/06/2017 09:02 AM, Slava Krutelyov wrote:
>>
>> ***@***.**** commented on this pull request.
>>
>>
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> In RecoLocalCalo/HcalRecProducers/src/HBHEPlan1Combiner.cc <#17313 (review)>:
>>
>> > +{
>> +public:
>> + explicit HBHEPlan1Combiner(const edm::ParameterSet&);
>> + ~HBHEPlan1Combiner();
>> +
>> + static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
>> +
>> +private:
>> + virtual void produce(edm::Event&, const edm::EventSetup&) override;
>> +
>> + //virtual void beginStream(edm::StreamID) override;
>> + //virtual void endStream() override;
>> + //virtual void beginRun(edm::Run const&, edm::EventSetup const&) override;
>> + //virtual void endRun(edm::Run const&, edm::EventSetup const&) override;
>> + //virtual void beginLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&) override;
>> + //virtual void endLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&) override;
>>
>> is this commented out code needed?
>> Please remove or add comments inline in the code why the commented out block is relevant
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub <#17313 (review)>, or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AFS4-Uobdr7yYdOXAMWsqgOujz8n4pi_ks5rZzYIgaJpZM4Lw4_h>.
>>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#17313 (comment)>, or
> mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEdcbqaMV4PNPX2bSo7nsNJqOmjxB_-Cks5rZz8WgaJpZM4Lw4_h>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#17313 (comment)>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFS4-el4-x6uoRi6ZStp0tD402NNIRX8ks5rZ0KfgaJpZM4Lw4_h>.
|
|
Pull request #17313 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
OK, done.
…On 02/06/2017 10:03 AM, Slava Krutelyov wrote:
***@***.**** commented on this pull request.
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
In RecoLocalCalo/HcalRecProducers/python/hbheplan1.py <#17313 (review)>:
> @@ -0,0 +1,21 @@
+import FWCore.ParameterSet.Config as cms
+
+hbheplan1 = cms.EDProducer(
sorry for possibly unclear message earlier: _cfi should stay in the file name.
Thank you.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#17313 (review)>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFS4-dhGMiqwtsHJgIdJvYwdUzt-oVVHks5rZ0Q8gaJpZM4Lw4_h>.
|
|
Pull request #17313 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
I agree.
…On 02/06/2017 09:57 AM, Kevin Pedro wrote:
@igv4321 <https://github.com/igv4321> I have not started working on the era yet. I will try to get to it today. I would prefer to have this and #17330 <#17330>
merged, and then make the PR creating the Era/scenario. At that time, we can fix any bugs that show up.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#17313 (comment)>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFS4-WmVD9fUDIALlXf_M9G0DUGaD7HRks5rZ0LzgaJpZM4Lw4_h>.
|
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
@igv4321 please check and clarify if aux fields are to be abandoned/ignored or if there is a meaningful way to combine. |
|
Yes, all effects that you observe are intended. hbhereco has 0 for timeFalling_ because timeFalling_ is used to store TDC time of QIE11 rechit. This time is not simulated in MC currently. The combined time will be driven to -120 in case either all QIE11 TDC times are "special" (underflow/overflow -- but this you can't observe with MC) or the rechit energy is not positive (no way to calculate energy-weighted TDC average). auxHBHE_ and aux_ are not filled because, in the original rechits, they are used to store raw ADC counts. These counts are used for various diagnostic purposes. However, there appears to be no meaningful way to combine them. hbheplan1 time is set to -999 if the energy is not positive. We probably don't care much about these rechits (and, again, there is no good way to calculate energy-weighted time average). |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1 |
Initial implementation of Plan 1 rechit combination. Discussed at
https://indico.cern.ch/event/602307/contributions/2430302/attachments/1394590/2125473/notes.pdf
This PR is not supposed to modify any existing results. The testing
will be performed when the Plan 1 geometry becomes up to snuff.