HCAL TP for Plan 1 #17540
HCAL TP for Plan 1 #17540
Conversation
The current code crashes for Phase2. Instead of generating all possible DetIds and testing which ones are allowed, it is a lot cleaner to just iterate over the produced metadata ids.
|
A new Pull Request was created by @matz-e (Matthias Wolf) for CMSSW_9_0_X. It involves the following packages: CalibCalorimetry/CaloTPG @ghellwig, @civanch, @Dr15Jones, @arunhep, @ianna, @mdhildreth, @cmsbuild, @rekovic, @franzoni, @cerminar, @mmusich, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
| return QIE10_OUTPUT_LUT_SIZE; | ||
| case HcalTopologyMode::TriggerMode_2017plan1: | ||
| if (theTopology->dddConstants()->isPlan1(id)) | ||
| return QIE11_OUTPUT_LUT_SIZE; |
There was a problem hiding this comment.
hello @matz-e
detIdSp_ https://github.com/matz-e/cmssw/blob/9c7c69d4203cf4d932921a4d4103956b8dc7cfbd/Geometry/HcalCommonData/interface/HcalDDDRecConstants.h#L136 is empty by construction, for now
which in turns means isPlan1(...) always returns false.
The logic in this switch looks good,
however, QIE11_OUTPUT_LUT_SIZE won't ever be returned.
There was a problem hiding this comment.
Doesn't it get filled here?
|
Comparison job queued. |
|
+1 |
|
+1 |
|
+1 |
|
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 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
| } // for iphi | ||
| }// for subdet | ||
| for (const auto& id: metadata->getAllChannels()) { | ||
| if (not (id.det() == DetId::Hcal and topo_->valid(id))) |
There was a problem hiding this comment.
is this if statement ever false in normal conditions? Eg, if its true is that a sign of corrupt data and/or incomplete conditions?
There was a problem hiding this comment.
When I implemented this, there were IDs that were either invalid HCAL IDs or not allowed by the topology… I'm not sure anymore which IDs exactly are skipped by this.
There was a problem hiding this comment.
indeed there are such IDs. Verification of validity by topology supposed to be a temporary work-around, I think.
There was a problem hiding this comment.
ok, thanks - is a dangerous condition to leave around, so lets try to clean up after than plan1 issues are behind us
There was a problem hiding this comment.
@matz-e Was this not the fix when simHCALTriggerPrimitives in sequence for Phase2?
There was a problem hiding this comment.
Yes, that was included in there, too. It somehow got reverted with the reversal of one of the L1T PRs, I think.
|
+1 |
Contains several previous PR, fixed:
TriggerModemeaning of the enumeration values unchanged.@davidlange6 I fixed the RelVal 10826.0 by keeping the meaning of the trigger mode consistent, I had added the new 2016 and Plan 1 definition and changed the underlying values, which had already been saved in the geometry DB.