Correct MTDGeomDet type problems #28590

dan131riley · 2019-12-09T21:59:43Z

PR description:

The BaseTrackerRecHit constructor implicitly assumes that the GeomDet passed to it is a TrackerGeomDet by doing a static_cast to that type:

cmssw/DataFormats/TrackerRecHit2D/interface/BaseTrackerRecHit.h

Line 29 in 30a2ff8

    
           LocalError lape = static_cast<TrackerGeomDet const*>(det())->localAlignmentError();

Changing the static_cast to dynamic_cast revealed that this assumption is violated by the MTDTrackingRecHitProducer, which passes an MTDGeomDet here:

cmssw/RecoLocalFastTime/FTLRecProducers/plugins/MTDTrackingRecHitProducer.cc

Line 120 in 30a2ff8

MTDTrackingRecHit hit(lp, le, *genericDet, cluster);

MTDGeomDet does not inherit from TrackerGeomDet, but is instead a copy&paste copy of TrackerGeomDet that inherits directly from GeomDet:

cmssw/Geometry/CommonDetUnit/interface/MTDGeomDet.h

Line 6 in 30a2ff8

class MTDGeomDet : public GeomDet {

This doesn't crash and isn't caught by the address sanitizer because the class layout is the same, but it is still a bug that could eventually lead to failures if the different classes evolve separately. Since MTDGeomDet currently has functionality identical to TrackerGeomDet, this PR makes MTDGeomDet a typedef for TrackerGeomDet. If in the future additional functionality is needed in MTDGeomDet it can be made a class that inherits from TrackerGeomDet. This resolves issue #28571.

In addition, this PR also removes some casts that are unnecessary since GeomDet and GeomDetUnit are now the same type, and makes a small change to AlignmentPositionError in GeomDet and TrackerGeomDet to improve encapsulation and reduce code replication.

PR validation:

A limited matrix was run with no new failures observed. This is strictly a technical fix that should have no effect on physics results.

… of the static pointer casts in TrackerRecHit2D into dynamic reference casts

…ed, remove some code duplication

cmsbuild · 2019-12-09T22:00:14Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-12-09T22:05:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28590/13086

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/TrackerRecHit2D/interface/BaseTrackerRecHit.h modified in PR(s): Integrating VectorHits reconstruction for the Phase2 OT #28101

cmsbuild · 2019-12-09T22:06:12Z

A new Pull Request was created by @dan131riley (Dan Riley) for master.

It involves the following packages:

DataFormats/TrackerRecHit2D
Geometry/CommonTopologies
Geometry/MTDGeometryBuilder
RecoLocalFastTime/FTLRecProducers

@perrotta, @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @gpetruc, @ebrondol, @rovere, @VinInn this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

kpedro88 · 2019-12-10T00:24:02Z

please test

kpedro88 · 2019-12-10T00:24:25Z

Do the additional dynamic_casts have a noticeable impact on CPU performance?

cmsbuild · 2019-12-10T00:24:30Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3892/console Started: 2019/12/10 01:27

cmsbuild · 2019-12-10T03:09:28Z

+1
Tested at: 89a2676
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1f990a/3892/summary.html

cmsbuild · 2019-12-10T03:09:31Z

Comparison job queued.

cmsbuild · 2019-12-10T04:37:03Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2800084
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2799742
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

cmsbuild · 2019-12-11T17:55:43Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3928/console Started: 2019/12/11 18:56

cmsbuild · 2019-12-11T21:25:17Z

+1
Tested at: 6c57594
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1f990a/3928/summary.html

cmsbuild · 2019-12-11T21:25:20Z

Comparison job queued.

cmsbuild · 2019-12-11T23:05:10Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2800084
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2799742
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

cvuosalo · 2019-12-12T23:19:26Z

+1

kpedro88 · 2019-12-13T21:12:53Z

+upgrade

perrotta · 2019-12-27T10:27:34Z

+1

The bug described in the PR description is fixed by assigning

using MTDGeomDet = TrackerGeomDet;

in Geometry/CommonTopologies/interface/MTDGeomDet.h

Other reco related updates in this PR are mostly cleanings
Jenkins tests pass and show no differences

cmsbuild · 2019-12-27T10:27:54Z

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 now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos, @silviodonato (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2019-12-27T10:29:40Z

@dan131riley please remove the mention to the move from static_cast's to dynamic_cast's which is still written in the PR description, for a correct reference

dan131riley · 2020-01-01T15:09:04Z

@perrotta i updated the PR description and resolved the remaining conversation

silviodonato · 2020-01-07T19:32:16Z

+1

Dan Riley added 2 commits December 6, 2019 14:21

Change the MTDGeomDet class to an alias for TrackerGeomDet; make most…

43a0a97

… of the static pointer casts in TrackerRecHit2D into dynamic reference casts

revert some unnecessary changes, remove casts that are no longer need…

89a2676

…ed, remove some code duplication

cmsbuild added this to the CMSSW_11_1_X milestone Dec 9, 2019

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Dec 9, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2019

cmsbuild added tests-started and removed tests-pending labels Dec 10, 2019

cmsbuild added tests-approved and removed tests-started labels Dec 10, 2019

cmsbuild added comparison-available and removed comparison-pending labels Dec 10, 2019

cmsbuild modified the milestones: CMSSW_11_1_X, CMSSW_11_0_X Dec 10, 2019

cmsbuild added tests-started and removed tests-pending labels Dec 11, 2019

cmsbuild added tests-approved and removed tests-started labels Dec 11, 2019

cmsbuild added comparison-available and removed comparison-pending labels Dec 11, 2019

cmsbuild added geometry-approved and removed geometry-pending labels Dec 12, 2019

cmsbuild added upgrade-approved and removed upgrade-pending labels Dec 13, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Dec 27, 2019

cmsbuild added orp-approved and removed orp-pending labels Jan 7, 2020

cmsbuild merged commit 2dd52eb into cms-sw:master Jan 7, 2020

dan131riley deleted the GeomDet-type-fixes branch January 7, 2020 20:52

perrotta mentioned this pull request Jan 13, 2020

MTDGeomDet type problem #28571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct MTDGeomDet type problems #28590

Correct MTDGeomDet type problems #28590

dan131riley commented Dec 9, 2019 •

edited

cmsbuild commented Dec 9, 2019

cmsbuild commented Dec 9, 2019

cmsbuild commented Dec 9, 2019

kpedro88 commented Dec 10, 2019

kpedro88 commented Dec 10, 2019

cmsbuild commented Dec 10, 2019 •

edited

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 11, 2019 •

edited

cmsbuild commented Dec 11, 2019

cmsbuild commented Dec 11, 2019

cmsbuild commented Dec 11, 2019

cvuosalo commented Dec 12, 2019

kpedro88 commented Dec 13, 2019

perrotta commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

perrotta commented Dec 27, 2019

dan131riley commented Jan 1, 2020

silviodonato commented Jan 7, 2020

Correct MTDGeomDet type problems #28590

Correct MTDGeomDet type problems #28590

Conversation

dan131riley commented Dec 9, 2019 • edited

PR description:

PR validation:

cmsbuild commented Dec 9, 2019

cmsbuild commented Dec 9, 2019

cmsbuild commented Dec 9, 2019

kpedro88 commented Dec 10, 2019

kpedro88 commented Dec 10, 2019

cmsbuild commented Dec 10, 2019 • edited

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 10, 2019

cmsbuild commented Dec 11, 2019 • edited

cmsbuild commented Dec 11, 2019

cmsbuild commented Dec 11, 2019

cmsbuild commented Dec 11, 2019

cvuosalo commented Dec 12, 2019

kpedro88 commented Dec 13, 2019

perrotta commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

perrotta commented Dec 27, 2019

dan131riley commented Jan 1, 2020

silviodonato commented Jan 7, 2020

dan131riley commented Dec 9, 2019 •

edited

cmsbuild commented Dec 10, 2019 •

edited

cmsbuild commented Dec 11, 2019 •

edited