Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HitPattern::stripHitFilter() #25692

Merged
merged 2 commits into from Jan 24, 2019

Conversation

makortel
Copy link
Contributor

The HitPattern::stripHitFilter() was counting the MTD hits as well. This PR limits the function to only the strip detectors.

The problem was found with clang (essentially) complaining the unit test using incorrect overload for HitPattern::appendHit(), and adding asserts in addition to the printouts (that would have indicated the problem of wrong overload already in #9959, and the need for an update in #24419).

Tested in CMSSW_10_5_X_2019-01-13-0000, not sure if there are changes in downstream.

@mtosi @JanFSchulte @VinInn

After moving to the interface with TrackerTopology, this code ended up
using the
appendHit(const uint16_t pattern, TrackingRecHit::Type hitType)
overload. The problem was found with clang.
Fixes the failing unit test.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25692/8062

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

DataFormats/TrackReco

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@gpetruc, @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

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 17, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32680/console Started: 2019/01/17 20:16

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2019

@makortel
just to make sure: does this issue affect only tracks with MTD hits?

@makortel
Copy link
Contributor Author

@slava77

just to make sure: does this issue affect only tracks with MTD hits?

I think so.

@perrotta
Copy link
Contributor

assign upgrade

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@kpedro88
Copy link
Contributor

@lgray FYI

@slava77
Copy link
Contributor

slava77 commented Jan 18, 2019

@makortel @VinInn @mtosi @JanFSchulte
I guess the OT hits are counted as strips.
Is this correct?

Perhaps someone should put a similar picture for the phase-2 tracker in

// +------------+---------------+---------------------------+-----------------+----------------+
// | tk/mu/mtd | sub-structure | sub-sub-structure | stereo | hit type |
// +------------+---------------+---------------------------+-----------------+----------------+
// | 11-10 | 9 8 7 | 6 5 4 3 | 2 | 1 0 | bit
// +------------+---------------+---------------------------+-----------------+----------------|
// | tk = 1 | PXB = 1 | layer = 1-3 | | hit type = 0-3 |
// | tk = 1 | PXF = 2 | disk = 1-2 | | hit type = 0-3 |
// | tk = 1 | TIB = 3 | layer = 1-4 | 0=rphi,1=stereo | hit type = 0-3 |
// | tk = 1 | TID = 4 | wheel = 1-3 | 0=rphi,1=stereo | hit type = 0-3 |
// | tk = 1 | TOB = 5 | layer = 1-6 | 0=rphi,1=stereo | hit type = 0-3 |
// | tk = 1 | TEC = 6 | wheel = 1-9 | 0=rphi,1=stereo | hit type = 0-3 |
// | mu = 0 | DT = 1 | 4*(stat-1)+superlayer | | hit type = 0-3 |
// | mu = 0 | CSC = 2 | 4*(stat-1)+(ring-1) | | hit type = 0-3 |
// | mu = 0 | RPC = 3 | 4*(stat-1)+2*layer+region | | hit type = 0-3 |
// | mu = 0 | GEM = 4 | 2*(stat-1)+2*(layer-1) | | hit type = 0-3 |
// | mu = 0 | ME0 = 5 | roll | | hit type = 0-3 |
// | mtd = 2 | BTL = 1 | moduleType = 1-3 | | hit type = 0-3 |
// | mtd = 2 | ETL = 2 | ring = 1-12 | | hit type = 0-3 |
// +------------+---------------+---------------------------+-----------------+----------------+

@makortel
Copy link
Contributor Author

I guess the OT hits are counted as strips.
Is this correct?

Yes, the phase2 OT barrel is treated as TOB and endcap as TID as noted in
https://github.com/cms-sw/cmssw/tree/master/Geometry/TrackerNumberingBuilder#phase-2-upgrade-detector-detid-schema-flat-or-tilted-geometry-with-new-pixel-detector-for-tdr-from-version-v4021

@kpedro88
Copy link
Contributor

please test
(previous round got stuck)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 21, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32749/console Started: 2019/01/21 18:33

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 22, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32777/console Started: 2019/01/22 18:17

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3097440
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3097242
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@kpedro88
Copy link
Contributor

+upgrade

@slava77
Copy link
Contributor

slava77 commented Jan 23, 2019

+1

for #25692 b2554f7

  • bugfix, as described.
    • This should affect the recently added tracks extended with MTD (phase-2); there is no monitoring for that yet to notice any difference.
  • jenkins tests pass and comparisons with the baseline show no differences

@cmsbuild
Copy link
Contributor

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 (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3739934 into cms-sw:master Jan 24, 2019
@perrotta
Copy link
Contributor

This probably needs a backport for 10_4_X, isn't it?

@makortel
Copy link
Contributor Author

@perrotta There you go #25767.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants