CTPPS: new DataFormats for local TOTEM RP reconstruction #14134

jan-kaspar · 2016-04-19T09:39:30Z

This pull request will be followed with another one containing the corresponding producers which are, for the moment, available here:
https://github.com/CTPPS/ctpps-offline/tree/develop/RecoLocalCTPPS/TotemRP

cmsbuild · 2016-04-19T09:39:52Z

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

It involves the following packages:

DataFormats/CTPPSReco

The following packages do not have a category, yet:

DataFormats/CTPPSReco

@cmsbuild, @davidlange6 can you please review it and eventually sign? Thanks.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

slava77 · 2016-04-19T12:11:25Z

@cmsbuild please test

cmsbuild · 2016-04-19T12:11:55Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12475/console

cmsbuild · 2016-04-19T12:51:51Z

Pull request #14134 was updated. @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

cmsbuild · 2016-04-19T15:29:55Z

+1
Tested at: f8a7ef2
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14134/12475/summary.html

cmsbuild · 2016-04-19T17:48:07Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14134/12475/summary.html

slava77 · 2016-04-20T21:20:58Z

DataFormats/CTPPSReco/interface/TotemRPLocalTrack.h

+
+        virtual ~FittedRecHit() {}
+
+        inline const TVector3 & getGlobalCoordinates() const { return space_point_on_det_; }


We have rather limited use of TVector* classes.
math::XYZPoint (for generically globally defined cases) or GlobalPoint (for interfaces with global<->local transformations)

slava77 · 2016-04-20T21:49:15Z

DataFormats/CTPPSReco/interface/TotemRPRecHit.h

+  if (l.getPosition() > r.getPosition())
+    return false;
+
+  if (l.getSigma() < r.getSigma())


can you really have multiple hits in the same detector that start at the same position?

slava77 · 2016-04-20T22:09:38Z

I don't see anything particularly egregious, mainly from a perspective that for some time digis are expected to be saved and that all hit reco and tracking can be redone very quickly.
So, the current classes can be expected to be reorganized at little cost to the past saved data to move to something more common with other CMS classes where appropriate.

jan-kaspar · 2016-04-21T10:48:14Z

@slava77 What you suggest often makes sense however it is quite complete redesign. We don't have time for it now but I am happy to do it when we put in place the SW infrastructure for all detectors of CTPPS (pixels, timing, ...). I've noted

do not use TVector*
use cm instead of mm
use TrackingRecHit, TrackBase

Regarding DetSetVector<FittedRecHit> track_hits_vector_ it was a misunderstanding. I had a feeling that you had a preference for DetSetVectors which you supported at the last meeting. We actually had DetIds stored in RecHits and I had to remove it and switch to a DetSetVector-compatible structure.

It is very rare to have multiple hits from one sensors associated with a track. It can only happen when, for some reason, there is active-inactive-active strip pattern. Then the two active strips may contribute to one track.

slava77 · 2016-04-21T10:57:28Z

On 4/21/16 12:48 PM, jan-kaspar wrote:

@slava77 https://github.com/slava77 What you suggest often makes sense
however it is quite complete redesign. We don't have time for it now but
I am happy to do it when we put in place the SW infrastructure for all
detectors of CTPPS (pixels, timing, ...). I've noted

do not use TVector*

This should be the easiest

use cm instead of mm

a bit more work, maybe

use TrackingRecHit, TrackBase

This will indeed need quite a bit of work.

Regarding |DetSetVector track_hits_vector_| it was a
misunderstanding. I had a feeling that you had a preference for
DetSetVectors which you supported at the last meeting.

This is true only where it is important: inclusive collections of digis
or hits
when multiple entries per Det are expected make sense.

Using DetSetVector to keep hits on a track trajectory is not practical
and can lead to loss of information about hit order in the fit.

We actually had
DetIds stored in RecHits and I had to remove it and switch to a
DetSetVector-compatible structure.

It is very rare to have multiple hits from one sensors associated with a
track. It can only happen when, for some reason, there is
active-inactive-active strip pattern. Then the two active strips may
contribute to one track.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14134 (comment)

jan-kaspar · 2016-04-21T11:28:29Z

@slava77 OK. As I wrote already, in near future we will need to add more detectors (pixels, timing, ...) to the SW design. In this moment, we can take into account all your comments and do it properly. Now, do you object using the classes in this PR?

slava77 · 2016-04-21T12:47:07Z

On 4/21/16 1:28 PM, jan-kaspar wrote:

@slava77 https://github.com/slava77 OK. As I wrote already, in near
future we will need to add more detectors (pixels, timing, ...) to the
SW design. In this moment, we can take into account all your comments
and do it properly. Now, do you object using the classes in this PR?

Can you point me to a file with results of the algorithms, something
with somewhat realistic physics inputs.

Thanks.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14134 (comment)

jan-kaspar · 2016-04-21T12:58:22Z

Can you point me to a file with results of the algorithms, something with somewhat realistic physics inputs.

Here:
http://jkaspar.web.cern.ch/jkaspar/slava/
is the output of
https://github.com/CTPPS/ctpps-offline/blob/develop/raw_data_chain_test.py
which uses TOTEM-standalone data from 2015. Let me know should it be not sufficient.

slava77 · 2016-04-21T20:40:16Z

DataFormats/CTPPSReco/interface/TotemRPLocalTrack.h

+    static const int dimension = 4;
+
+    ///< covariance matrix size
+    static const int covarianceSize = dimension * dimension;


this is supposed to be a symmetric matrix. Why covarianceSize^2 ?

slava77 · 2016-04-21T20:51:13Z

+1

for #14134 f8a7ef2 

code is in line with the description; I still expect this to evolve
jenkins test pass, more pertinent here: it compiles and there are no issues from the static analysis
based on the reference file, I see that the digis are about a factor of 10 smaller than the downstream combination of hits/clusters/patterns/and tracks. The ultimate in complexity is TotemRPLocalTrack that takes up 2/3 of the post-digi TOTEM products size: double precision and generic 4x4 covariance are partly to blame. If occupancies in pp collisions are higher than in this reference file, we will not be able (or, rather not) to save the TotemRPLocalTrack and instead assume all higher level reco is to be redone at analysis level from digis.

cmsbuild · 2016-04-21T20:52:15Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

jan-kaspar · 2016-04-22T08:57:22Z

Thanks, Slava!

With the current SW, we can't resolve more than 1 (local) track per RP per event. Hence even if hit occupancy grows, there will be no more track objects.

Size-wise, the actual design is not optimal from yet another perspective: all higher level objects keep an independent copy of the associated RecHits. It would be better to store a pointer/reference instead. Is there a sort of persistent reference suitable for such a use?

slava77 · 2016-04-22T10:21:03Z

On 4/22/16 10:57 AM, jan-kaspar wrote:

Thanks, Slava!

With the current SW, we can't resolve more than 1 (local) track per RP
per event. Hence even if hit occupancy grows, there will be no more
track objects.

Size-wise, the actual design is not optimal from yet another
perspective: all higher level objects keep an independent copy of the
associated RecHits. It would be better to store a pointer/reference
instead. Is there a sort of persistent reference suitable for such a use?

DetSeReftVector should work to reference hits in the DetSetVector

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14134 (comment)

jan-kaspar · 2016-04-22T10:43:47Z

On Fri, Apr 22, 2016 at 12:21 PM, Slava Krutelyov
notifications@github.com wrote:

On 4/22/16 10:57 AM, jan-kaspar wrote:

Thanks, Slava!

With the current SW, we can't resolve more than 1 (local) track per RP
per event. Hence even if hit occupancy grows, there will be no more
track objects.

Size-wise, the actual design is not optimal from yet another
perspective: all higher level objects keep an independent copy of the
associated RecHits. It would be better to store a pointer/reference
instead. Is there a sort of persistent reference suitable for such a use?

DetSeReftVector should work to reference hits in the DetSetVector

Great, thanks! I'll keep this in mind when (re-)designing the classes for CTPPS.

CTPPS: new DataFormats for local TOTEM RP reconstruction, backport of #14134

Initial import from the CTPPS repository.

f8a7ef2

cmsbuild added this to the Next CMSSW_8_1_X milestone Apr 19, 2016

cmsbuild added pending-signatures tests-pending orp-pending new-package-pending comparison-pending labels Apr 19, 2016

slava77 mentioned this pull request Apr 19, 2016

add DataFormats/CTPPSReco to reco category cms-sw/cms-bot#663

Merged

cmsbuild added tests-started reconstruction-pending and removed tests-pending new-package-pending labels Apr 19, 2016

cmsbuild added tests-approved and removed tests-started labels Apr 19, 2016

cmsbuild added comparison-available and removed comparison-pending labels Apr 19, 2016

jan-kaspar mentioned this pull request Apr 20, 2016

CTPPS: DQM for TOTEM RP #14034

Merged

slava77 reviewed Apr 20, 2016
View reviewed changes

slava77 reviewed Apr 21, 2016
View reviewed changes

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Apr 21, 2016

davidlange6 merged commit 4dcbe29 into cms-sw:CMSSW_8_1_X Apr 22, 2016

jan-kaspar deleted the ctpps_rp_reco_data_formats branch April 22, 2016 08:57

grzanka mentioned this pull request Apr 25, 2016

CTPPS new reconstruction geometry related to TOTEM RP detectors #14029

Merged

grzanka mentioned this pull request May 19, 2016

CTPPS: new DataFormats for local TOTEM RP reconstruction, backport of #14134 #14580

Merged

cmsbuild added a commit that referenced this pull request May 24, 2016

Merge pull request #14580 from CTPPS/ctpps_rp_reco_data_formats_backport

0574bf0

CTPPS: new DataFormats for local TOTEM RP reconstruction, backport of #14134

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTPPS: new DataFormats for local TOTEM RP reconstruction #14134

CTPPS: new DataFormats for local TOTEM RP reconstruction #14134

jan-kaspar commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

slava77 commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

slava77 Apr 20, 2016

slava77 Apr 20, 2016

slava77 commented Apr 20, 2016

jan-kaspar commented Apr 21, 2016

slava77 commented Apr 21, 2016

jan-kaspar commented Apr 21, 2016

slava77 commented Apr 21, 2016

jan-kaspar commented Apr 21, 2016

slava77 Apr 21, 2016

slava77 commented Apr 21, 2016

cmsbuild commented Apr 21, 2016

jan-kaspar commented Apr 22, 2016

slava77 commented Apr 22, 2016

jan-kaspar commented Apr 22, 2016


		virtual ~FittedRecHit() {}

		inline const TVector3 & getGlobalCoordinates() const { return space_point_on_det_; }

CTPPS: new DataFormats for local TOTEM RP reconstruction #14134

CTPPS: new DataFormats for local TOTEM RP reconstruction #14134

Conversation

jan-kaspar commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

slava77 commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

cmsbuild commented Apr 19, 2016

slava77 Apr 20, 2016

Choose a reason for hiding this comment

slava77 Apr 20, 2016

Choose a reason for hiding this comment

slava77 commented Apr 20, 2016

jan-kaspar commented Apr 21, 2016

slava77 commented Apr 21, 2016

jan-kaspar commented Apr 21, 2016

slava77 commented Apr 21, 2016

jan-kaspar commented Apr 21, 2016

slava77 Apr 21, 2016

Choose a reason for hiding this comment

slava77 commented Apr 21, 2016

cmsbuild commented Apr 21, 2016

jan-kaspar commented Apr 22, 2016

slava77 commented Apr 22, 2016

jan-kaspar commented Apr 22, 2016