Totem Timing RecHit Producer #23101
Totem Timing RecHit Producer #23101
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23101/4520 |
|
A new Pull Request was created by @nminafra (Nicola Minafra) for master. It involves the following packages: DataFormats/CTPPSReco @perrotta, @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
The tests are being triggered in jenkins. |
|
@slava77 I've tested from raw and everything is fine: the interface of CTPPSDiamondRecHit didn't change. |
|
On 5/1/18 5:45 AM, Nicola Minafra wrote:
@slava77 <https://github.com/slava77> I've tested from raw and
everything is fine: the interface didn't change.
Then I've run the DQM in a release with the package changed as in this
PR, starting from AOD (with "old" rechits) and everything is fine.
This DQM test sounds good for the purpose.
Just to be sure, was the DQM job running only an analyzer?
AOD contains digis and I can imagine that reco modules could've just
remade the hits.
… Do you propose any other test?
Thanks
|
|
@slava77 Yes: For future reference, this is the test I used: |
|
-1 Tested at: 2a7296d The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log
I found errors in the following addon tests: cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_2016 : FAILED - time: date Tue May 1 15:36:35 2018-date Tue May 1 15:33:06 2018 s - exit: 17920 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
|
+1
|
|
@ianna : you already signed once this PR. Please signi it again, if you agree with the updates integrated since then |
|
+1 |
|
@nminafra do I understand correctly that this code is supposed to be valid for the whole 2018 data taking? Clearly, as it is modifying the event content, for its backport in 101X we will need to be careful to manage it though an era, so as not to interfere with the already existing setup. But I would like to understand the situation for 102X |
|
@fabiocos Unless required by you, we do not plan to remove this code. If the detector is not in use, the collections will not be created anyways, so it creates no harm to us. |
|
On 5/11/18 1:37 AM, Nicola Minafra wrote:
@fabiocos <https://github.com/fabiocos> Unless required by you, we do
not plan to remove this code. If the detector is not in use, the
collections will not be created anyways, so it creates no harm to us.
Actually, in this regard: I was planning to extend also
CTPPSDiamondLocalTracks
<https://github.com/cms-sw/cmssw/blob/master/DataFormats/CTPPSReco/interface/CTPPSDiamondLocalTrack.h>
in a similar way (mother class CTPPSTimingLocalTrack, inheriting
CTPPSDiamondLocalTrack and TotemTimingLocalTrack). I didn't do it
because I'm not sure when the track producer will be ready. However, if
you want, I can do it now in a way that, for the backport, both
changements can be done in the same era.
Thanks
There is going to be a rereco with 10_2_X.
All CTPPS rereco can still be redone by running on AOD.
So, we shouldn't push too hard on 10_1_X backport beyond the operational
needs for CTPPS to take good quality data during the upcoming special run.
BTW, regarding "If the detector is not in use, the collections will not
be created ": all configured reco producer modules should produce a
collection every event. It can be empty.
|
True! thanks for the clarification @slava77 |
|
@slava77 I am thinking to the strategy to be adopted for the 90 m beta* data taking of end of June, when the full 10_2_X will not still fully at production quality. |
|
+operations |
|
+1 |
cmsbuild
added
fully-signed
operations-approved
orp-approved
and removed
operations-pending
orp-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged. |
|
Thanks everyone for the review! |
|
On 5/14/18 11:23 AM, Fabio Cossutti wrote:
@slava77 <https://github.com/slava77> I am thinking to the strategy to
be adopted for the 90 m beta* data taking of end of June, when the full
10_2_X will not still fully at production quality.
—
What I meant is:
If the hits are needed just for analysis, their reconstruction can be
done on the fly from AOD files at analysis level.
So, we do not have to change 10_1_X production setup.
That's unless the rechits are required to take data (e.g. in the trigger)
|
This RP introduces recHits for TotemTiming detector in vertical Roman Pots (90m dedicated run).
NOTES:
NEXT IN LINE:
calibration values without changing the rest of the code. We aspect a small difference on the performance, tests still on going.
Many thanks for your review!
@forthommel