Energy threshold update for new MTD geometry #31654
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31654/18754
|
|
A new Pull Request was created by @gsorrentino18 (Giulia Sorrentino) for master. It involves the following packages: RecoLocalFastTime/FTLCommonAlgos @perrotta, @jpata, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| const MTDTimeCalib* time_calib_; | ||
| const MTDTopology* topology_; | ||
| static constexpr int topologycode1Disk_ = 4; |
There was a problem hiding this comment.
@gsorrentino18 @casarsa there is no need to define hardcoded thresholds, you may simply use the MTDTopologyMode enum class with something like
if (mtdTopologyMode <= static_cast<int>(MTDTopologyMode::Mode::barphiflat)) {
as we are doing elsewhere in the code (and I am using in the RecoMTD/DetLayers code I am preparing)
|
Pull request #31654 was updated. @perrotta, @civanch, @silviodonato, @mdhildreth, @cmsbuild, @franzoni, @kpedro88, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please check and sign again. |
|
@slava77 @bsunanda the latest commit by @gsorrentino18 adds the updated Phase2C11 era that we had discussed, this should go in the direction of the update suggested. At this point I should update #31765 to use this new Era for both D72 and D73 |
|
please test |
|
The tests are being triggered in jenkins.
|
| from Configuration.Eras.Era_Phase2C11_cff import Phase2C11 | ||
| from Configuration.Eras.Modifier_phase2_etlV4_cff import phase2_etlV4 | ||
|
|
||
| Phase2C11_etlV4 = cms.ModifierChain(Phase2C11, phase2_etlV4) |
There was a problem hiding this comment.
for top-level Eras, please follow the existing naming scheme:
e.g. C11 is a subdetector version for HGCal described in https://github.com/cms-sw/cmssw/tree/master/Configuration/Geometry, so use the corresponding subdetector version for MTD
There was a problem hiding this comment.
@kpedro88 as discussed above, there is no intention to add an MTD dedicated era, this is a temporary workaround while #31765 is integrated to have a variant of Phase2C11 where D72 and D73 can be exercised without problems. When that PR is integrated, there is no reason to have scenarios with PhaseC11 AND old ETL , Phase2C11 should include this modifier as a new default, and the era added here should just be retired. Do you see a problem with this? According to what I discussed with @bsunanda Phase2C11 should stay for a while, while the old ETL in conjunction with it should disappear asap
There was a problem hiding this comment.
@fabiocos thanks for clarifying. in this case, the temporary Era is acceptable as it currently is.
|
-1 Tested at: 98c389f CMSSW: CMSSW_11_2_X_2020-10-20-2300 I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test TestDQMOnlineClient-beam_dqm_sourceclient had ERRORS |
|
Comparison job queued. |
|
@qliphy @silviodonato the unit test error seems to me unrelated to this PR... |
|
Comparison is ready Comparison Summary:
|
There was a problem hiding this comment.
IIUC, there is a preference now to have this PR be merged in #31765
based on #31765 (comment)
the comments are minor enough to be ignored; still, if there is a chance to update, please pick these up
| phase2_etlV4.toModify(_endcapAlgo, thresholdToKeep = 0.005 ) | ||
| phase2_etlV4.toModify(_endcapAlgo, calibrationConstant = 0.001 ) |
There was a problem hiding this comment.
| phase2_etlV4.toModify(_endcapAlgo, thresholdToKeep = 0.005 ) | |
| phase2_etlV4.toModify(_endcapAlgo, calibrationConstant = 0.001 ) | |
| phase2_etlV4.toModify(_endcapAlgo, thresholdToKeep = 0.005, calibrationConstant = 0.001 ) |
is less verbose
|
|
||
|
|
There was a problem hiding this comment.
not necessary
|
@gsorrentino18 unless @slava77 @kpedro88 @silviodonato have a different advice, I would say that at this point this PR may just be closed |
|
-1 superseded by #31765 |
|
please close |
PR description:
The energy threshold parameter used in the MTDRecHitAlgo plugin has been modified in order to discriminate between the two different MTD ETL geometries (with one ETL disk and two ETL disks). The two MTD ETL geometries are identified via the MTDTopologyMode.
PR validation:
The new code has been tested in the release CMSSW_11_2_0_pre6.