CTPPS: miniAOD #17162

jan-kaspar · 2017-01-12T11:01:27Z

This PR brings CTPPS objects to the miniAOD level.

The new data format (for both tracking and timing detectors):
DataFormats/CTPPSReco/interface/CTPPSLocalTrackLite.h

The ED producer
RecoCTPPS/TotemRPLocal/plugins/CTPPSLocalTrackLiteProducer.cc
is part of the standard reconstruction sequence
RecoCTPPS/TotemRPLocal/python/totemRPLocalReconstruction_cff.py
and the produced data are also included in the AOD:
RecoCTPPS/Configuration/python/RecoCTPPS_EventContent_cff.py

The slimming config:
PhysicsTools/PatAlgos/python/slimming/MicroEventContent_cff.py
has been updated to keep the new data format.

cmsbuild · 2017-01-12T11:01:45Z

A new Pull Request was created by @jan-kaspar for CMSSW_9_0_X.

It involves the following packages:

DataFormats/CTPPSReco
PhysicsTools/PatAlgos
RecoCTPPS/Configuration
RecoCTPPS/TotemRPLocal

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @JyothsnaKomaragiri this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

slava77 · 2017-01-12T13:38:23Z

DataFormats/CTPPSReco/interface/CTPPSLocalTrackLite.h

+struct CTPPSLocalTrackLite
+{
+  public:
+    CTPPSLocalTrackLite(uint32_t pid=0, float px=0., float pxu=-1., float py=0., float pyu=-1., float pt=0., float ptu=-1.)


please define a proper default constructor (without arguments).
Are there use cases for constructors where the first 3-4 arguments are not known while the last few can have default values?

Switch to class from struct

slava77 · 2017-01-12T13:40:06Z

DataFormats/CTPPSReco/interface/CTPPSLocalTrackLite.h

+    {
+    }
+
+    uint32_t getRPId() const


please add doxygen-compatible comments with documentation of the member methods (member data has some comments already, nice).

slava77 · 2017-01-12T13:40:58Z

DataFormats/CTPPSReco/src/classes_def.xml

@@ -39,4 +39,11 @@
  <class name="edm::Wrapper<edm::DetSetVector<TotemRPLocalTrack::FittedRecHit>>"/>
  <class name="std::vector<edm::DetSet<TotemRPLocalTrack::FittedRecHit> >"/>
  <class name="std::vector<TotemRPLocalTrack::FittedRecHit>"/>
+
+  <class name="CTPPSLocalTrackLite"  ClassVersion="2">
+    <version ClassVersion="2" checksum="3838813906"/>


version 3 is preferred now for the starting point

…cumented, class version 3.

jan-kaspar · 2017-01-12T17:26:38Z

@slava77 I've implemented your suggestions.

Are there use cases for constructors where the first 3-4 arguments are not known while the last few can have default values?

Not really - the foreseen timing detectors always have some segmentation so at least coarse spatial information is always available.

slava77 · 2017-01-12T19:31:48Z

@cmsbuild please test

It's not quite clear to me if this Lite producer will work by running on AOD inputs
@jan-kaspar can you confirm?

cmsbuild · 2017-01-12T19:32:06Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17258/console Started: 2017/01/12 22:07

cmsbuild · 2017-01-12T22:05:47Z

-1

Tested at: b94427d

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17162/17258/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following worklfows:
136.731 step3

runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log

cmsbuild · 2017-01-12T22:05:50Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

slava77 · 2017-01-12T22:15:36Z

in step3 136.731

   [1] Running path 'dqmoffline_step'
   [2] Calling method for module TotemRPDQMSource/'totemRPDQMSource'
Exception Message:
CTPPSDetId ctor: Invalid parameters: arm=19812843 station=5 rp=2

I hope the next update is first tested with the standard runTheMatrix.py -l limited -i all

slava77 · 2017-01-17T13:39:13Z

@jan-kaspar
is this PR compatible with 80X?
If so, you may want to make a PR for 80X, in view of possible re-miniAOD

jan-kaspar · 2017-01-17T17:04:16Z

is this PR compatible with 80X?
If so, you may want to make a PR for 80X, in view of possible re-miniAOD

Yes to both. I typically issue back-ports only after the development PR has been approved - to avoid making the same corrections in multiple branches. Let me know if you think that I should do differently this time.

slava77 · 2017-01-17T17:18:23Z

On 1/17/17 9:04 AM, jan-kaspar wrote: is this PR compatible with 80X? If so, you may want to make a PR for 80X, in view of possible re-miniAOD Yes to both. I typically issue back-ports only after the development PR has been approved - to avoid making the same corrections in multiple branches. Let me know if you think that I should do differently this time.

Thanks for clarifying. I think we can proceed as you described, make a backport after this is signed.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbm6fJxHB4__07LHVQWNl-hzl_aCpks5rTPSRgaJpZM4Lhn34>.

slava77 · 2017-01-19T02:04:43Z

+1

for #17162 5853bfc

code changes are roughly in line with the description (there are also additional technical updates in DQM to use CTPPSDetId instead of unsigned int, apparently fixing up several spots missed in CTPPS: detector id update #16010 )
jenkins tests pass and comparisons show no differences (existing products and plots are not affected)
local test with 136.725 (2015B; the same run as what's in the shortMatrix in 137.731) had CTPPS data and shows some reasonable distributions for the new products (after the validate.C script is updated)

[red: baseline; black: with this PR, using CMSSW_9_0_X_2017-01-18-1100]

The new product branch appears in RECO/AOD and in miniAOD files with a small size, as expected

27        CTPPSLocalTrackLites_ctppsLocalTrackLiteProducer__reRECO

CPU time of the ctppsLocalTrackLiteProducer is negligible.

slava77 · 2017-01-20T14:07:09Z

@jan-kaspar
looks like it's time for the backport.

jan-kaspar · 2017-01-20T18:45:31Z

@slava77 OK, I've cherry-picked the commits from this PR to our "continuous integration" branch intended for the back-port preceeding the legacy re-reco:
https://github.com/CTPPS/cmssw/tree/ctpps_cumulative_backport_8_0_X

slava77 · 2017-01-20T18:55:41Z

On 1/20/17 10:45 AM, jan-kaspar wrote: @slava77 <https://github.com/slava77> OK, I've cherry-picked the commits from this PR to our "continuous integration" branch intended for the back-port preceeding the legacy re-reco: https://github.com/CTPPS/cmssw/tree/ctpps_cumulative_backport_8_0_X

OK for the rereco plans. Actually, in case we were going to have re-miniAOD release some time next week, I was going to suggest you add CTPPS to miniAOD to go with it. If you are interested, the backport will need to be minimal (no detid migration).

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbjM3bC6_dzEjb7GKLBvERHgwOy6rks5rUQDLgaJpZM4Lhn34>.

davidlange6 · 2017-01-23T19:13:26Z

Hi @jan-kaspar @slava77 -it looks like this PR is causing data workflows to crash - eg, 136.721. Could you have a look and provide a fix. Thanks @Dr15Jones

slava77 · 2017-01-23T19:53:18Z

On 1/23/17 11:13 AM, David Lange wrote: Hi @jan-kaspar <https://github.com/jan-kaspar> @slava77 <https://github.com/slava77> -it looks like this PR is causing data workflows to crash - eg, 136.721. Could you have a look and provide a fix. Thanks @Dr15Jones <https://github.com/Dr15Jones>

the crash is related to TotemRPGeometryESModule and TotemRPUVPatternFinder https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_9_0_X_2017-01-20-2300/pyRelValMatrixLogs/run/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbtCCzy2WNFeZDYuR_JlIwNt1bTGhks5rVPvXgaJpZM4Lhn34>.

slava77 · 2017-01-24T01:04:55Z

curiously, the crash in 136.731 happened in CMSSW_9_0_X_2017-01-20-2300
and is no longer visible in CMSSW_9_0_X_2017-01-23-1100

davidlange6 · 2017-01-24T01:08:56Z

but to clarify there are plenty of related ones with the same error..

…

On Jan 23, 2017, at 5:04 PM, Slava Krutelyov ***@***.***> wrote: curiously, the crash in 136.731 happened in CMSSW_9_0_X_2017-01-20-2300 and is no longer visible in CMSSW_9_0_X_2017-01-23-1100 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

jan-kaspar · 2017-01-24T10:37:11Z

Thanks Slava for sharing the link to the log. Reading through, I can't really understand the problem. I've run the workflow 136.731 within CMSSW_9_0_X_2017-01-20-2300 on Lxplus, but can't reproduce the problem. Would you have any hints how to debug it? Thanks!

slava77 · 2017-01-24T14:00:10Z

On 1/24/17 2:37 AM, jan-kaspar wrote: Thanks Slava for sharing the link to the log. Reading through, I can't really understand the problem. I've run the workflow 136.731 within CMSSW_9_0_X_2017-01-20-2300 on Lxplus, but can't reproduce the problem. Would you have any hints how to debug it? Thanks!

IB tests are done multithreaded runTheMatrix.py -l 136.731 -t 4 There is a static bool evRotationStoreState; which may be introducing a problem, not sure if anything else similarly non-threadsafe can be a problem.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbtPL-gpWSqzjgJPAgOyUYKTJpUEnks5rVdRXgaJpZM4Lhn34>.

slava77 · 2017-01-24T14:00:50Z

the static bool is in https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Geometry/VeryForwardGeometryBuilder/plugins/TotemRPGeometryESModule.cc#L180

jan-kaspar · 2017-01-24T18:46:23Z

Thanks Slava! Finally, with option "-j 16 -t 8" I can (sometimes) reproduce the problem. But the behaviour is quite random: sometimes the segfault occurs sometimes not, the crashing module is not always the same one...

I've tried to comment out code using the static variable in "TotemRPGeometryESModule" but it doesn't solve the problem.

Are we really sure that the problem comes from this PR? To me, it seems rather unrelated...

davidlange6 · 2017-01-24T19:04:49Z

we aren't - I can propose to back out this PR and see if the errors go away.

davidlange6 · 2017-01-25T19:33:38Z

to follow up here - yes, reverting this pull request fixes our problems.

dan131riley · 2017-01-25T21:49:13Z

AFAICT there's no reason for anything in MeasuredGeometryProducer to be static. It looks like some function got declared static for no good reason, and that led to having to make evRotationStoreState also static, but none of those need to be static, as they can't possibly be called without a "this".

dan131riley · 2017-01-25T23:39:52Z

There may be a conflict between TotemRPUVPatternFinder and RPCTrigger usage of Xerces, possibly a latent concurrency bug in RPCTrigger. I've opened issue #17288

jan-kaspar · 2017-01-26T11:05:19Z

to follow up here - yes, reverting this pull request fixes our problems.

Thanks David for checking this.

AFAICT there's no reason for anything in MeasuredGeometryProducer to be static. It looks like some function got declared static for no good reason, and that led to having to make evRotationStoreState also static, but none of those need to be static, as they can't possibly be called without a "this".

Right. The entire code of MeasuredGeometryProducer is ugly and at some point I'd like to replace it with something more standard. In a short term - if we like - it can be completely commented out. In the current workflow (unlikely to change in short future), alignment corrections are only applied at analysis level. Therefore no alignment corrections during RECO and no meanigful use of MeasuredGeometryProducer.

There may be a conflict between TotemRPUVPatternFinder and RPCTrigger usage of Xerces, possibly a latent concurrency bug in RPCTrigger. I've opened issue #17288

Many thanks for investigating Dan! Do I interpret correctly the discussion that the problem most likely lies in RPCTrigger?

I don't quite understand the possible interference between TotemRPUVPatternFinder and RPCTrigger since I don't think that Xerces is actually run by the CTPPS code. In steps: TotemRPUVPatternFinder does not use Xerces, it only requires geometry from EventSetup:
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/RecoCTPPS/TotemRPLocal/plugins/TotemRPUVPatternFinder.cc#L170
The geometry builder "TotemRPGeometryESModule" would lead to Xerces use if there were alignment XML files to load. But in the current configuration there are none. So the only way how TotemRPGeometryESModule could possibly lead to use of Xerces is via requesting the geometry from the DDL files. Is Xerces used for that?

Also, I don't really understand why this PR makes this problem appear. The only thing changed in TotemRPUVPatternFinder was the interpretation of detector ids - seemingly very unrelated.

dan131riley · 2017-01-26T18:55:06Z

On Jan 26, 2017, at 06:05, jan-kaspar ***@***.***> wrote: I don't quite understand the possible interference between TotemRPUVPatternFinder and RPCTrigger since I don't think that Xerces is actually run by the CTPPS code. In steps: TotemRPUVPatternFinder does not use Xerces, it only requires geometry from EventSetup: https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/RecoCTPPS/TotemRPLocal/plugins/TotemRPUVPatternFinder.cc#L170 The geometry builder "TotemRPGeometryESModule" would lead to Xerces use if there were alignment XML files to load. But in the current configuration there are none. So the only way how TotemRPGeometryESModule could possibly lead to use of Xerces is via requesting the geometry from the DDL files. Is Xerces used for that?

Yes, Xerces is used for the DDL files. The simplified call stack is something like TotemRPUVPatternFinder::produce() asks for the TotemRPGeometry which calls TotemRPGeometryESModule::produceRealTG TotemRPGeometryESModule::produceRealGD TotemRPGeometryESModule::produceMeasuredGD TotemRPGeometryESModule::produceMeasuredDDCV That finally gets the DDCompactView via XMLIdealGeometryESSource::produce which calls DDLParser::parse which then calls into Xerces to parse it.

Also, I don't really understand why this PR makes this problem appear. The only thing changed in TotemRPUVPatternFinder was the interpretation of detector ids - seemingly very unrelated.

It probably isn't due to any change in TotemRPUVPatternFinder, but rather comes from a change in when (or if) it gets called. If the problem is due to interference between TotemRPUVPatternFinder and RPCTrigger, then it will only happen when those initializations overlap. So maybe some change in that PR (like CTPPSLocalTrackLiteProducer doing a "consumes<DetSetVector<TotemRPLocalTrack>>") changes the producer schedule in a way that makes it very likely that TotemRPUVPatternFinder and RPCTrigger overlap. Looking at how that PR changed the product dependency graph might tell us. Concurrency bugs are fun that way...

jan-kaspar added 6 commits December 20, 2016 20:38

CTPPSDetId scheme used consistently everywhere.

e4d972d

Added new data format: CTPPSLocalTrackLite.

c69c501

Added new producer: CTPPSLocalTrackLiteProducer.

09fa8a5

CTPPSLocalTrackLiteProducer added to local reco sequence.

66f8d76

CTPPSLocalTrackLite added to FEVT, RECO and AOD event content.

acff909

CTPPSLocalTrackLite added to miniAOD event content.

22bfa1c

cmsbuild added this to the Next CMSSW_9_0_X milestone Jan 12, 2017

cmsbuild added analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 12, 2017

slava77 reviewed Jan 12, 2017

View reviewed changes

Class instead of struct, added default constructor, member methods do…

b94427d

…cumented, class version 3.

cmsbuild added tests-started and removed tests-pending labels Jan 12, 2017

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Jan 12, 2017

cmsbuild added the dqm-approved label Jan 16, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 19, 2017

davidlange6 merged commit 4a12ea8 into cms-sw:CMSSW_9_0_X Jan 20, 2017

davidlange6 mentioned this pull request Jan 24, 2017

Revert "CTPPS: miniAOD" #17265

Merged

jan-kaspar mentioned this pull request Jan 25, 2017

CTPPS: miniAOD (minimal back-port of #17162) #17282

Closed

jan-kaspar mentioned this pull request Feb 6, 2017

CTPPS: miniAOD (resubmitted) #17424

Merged

slava77 mentioned this pull request Feb 14, 2017

CTPPS: diamonds geometry and RecHits #17317

Closed

jan-kaspar deleted the ctpps_miniaod branch February 27, 2017 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTPPS: miniAOD #17162

CTPPS: miniAOD #17162

jan-kaspar commented Jan 12, 2017

cmsbuild commented Jan 12, 2017

slava77 Jan 12, 2017

slava77 Jan 12, 2017

slava77 Jan 12, 2017

jan-kaspar commented Jan 12, 2017

slava77 commented Jan 12, 2017

cmsbuild commented Jan 12, 2017 •

edited

cmsbuild commented Jan 12, 2017

cmsbuild commented Jan 12, 2017

slava77 commented Jan 12, 2017

slava77 commented Jan 17, 2017

jan-kaspar commented Jan 17, 2017

slava77 commented Jan 17, 2017 via email

slava77 commented Jan 19, 2017

slava77 commented Jan 20, 2017

jan-kaspar commented Jan 20, 2017

slava77 commented Jan 20, 2017 via email

davidlange6 commented Jan 23, 2017

slava77 commented Jan 23, 2017 via email

slava77 commented Jan 24, 2017

davidlange6 commented Jan 24, 2017 via email

jan-kaspar commented Jan 24, 2017

slava77 commented Jan 24, 2017 via email

slava77 commented Jan 24, 2017

jan-kaspar commented Jan 24, 2017

davidlange6 commented Jan 24, 2017

davidlange6 commented Jan 25, 2017

dan131riley commented Jan 25, 2017

dan131riley commented Jan 25, 2017

jan-kaspar commented Jan 26, 2017

dan131riley commented Jan 26, 2017 via email

CTPPS: miniAOD #17162

CTPPS: miniAOD #17162

Conversation

jan-kaspar commented Jan 12, 2017

cmsbuild commented Jan 12, 2017

slava77 Jan 12, 2017

Choose a reason for hiding this comment

slava77 Jan 12, 2017

Choose a reason for hiding this comment

slava77 Jan 12, 2017

Choose a reason for hiding this comment

jan-kaspar commented Jan 12, 2017

slava77 commented Jan 12, 2017

cmsbuild commented Jan 12, 2017 • edited

cmsbuild commented Jan 12, 2017

cmsbuild commented Jan 12, 2017

slava77 commented Jan 12, 2017

slava77 commented Jan 17, 2017

jan-kaspar commented Jan 17, 2017

slava77 commented Jan 17, 2017 via email

slava77 commented Jan 19, 2017

slava77 commented Jan 20, 2017

jan-kaspar commented Jan 20, 2017

slava77 commented Jan 20, 2017 via email

davidlange6 commented Jan 23, 2017

slava77 commented Jan 23, 2017 via email

slava77 commented Jan 24, 2017

davidlange6 commented Jan 24, 2017 via email

jan-kaspar commented Jan 24, 2017

slava77 commented Jan 24, 2017 via email

slava77 commented Jan 24, 2017

jan-kaspar commented Jan 24, 2017

davidlange6 commented Jan 24, 2017

davidlange6 commented Jan 25, 2017

dan131riley commented Jan 25, 2017

dan131riley commented Jan 25, 2017

jan-kaspar commented Jan 26, 2017

dan131riley commented Jan 26, 2017 via email

cmsbuild commented Jan 12, 2017 •

edited