Merged the MeasurementEstimators into the PixelHitMatcher #28262

guitargeek · 2019-10-24T16:26:52Z

PR description:

After my last PR #28213 definitely removed the support in the electron pixel hit matching for not using the standard tracker seeds, there is still some code left to remove which is now obsolete. Much of it could be found in the classes BarrelMeasurementEstimator and ForwardMeasurementEstimator, which unnecessarily inherit from MeasurementEstimator and only one of their respective member functions is actually used. Whether a track extrapolation falls into the matching window in the next layer.

I propose to integrate the remaining functionality of these estimator classes after cleaning them up as private functors of the PixelHitMatcher class. Sorry for having multiple of these small cleanup PRs, but I don't want to risk any differences in the comparisons by doing to much at once and possibly making mistakes.

PR validation:

CMSSW compiles and local matrix tests pass.

if this PR is a backport please specify the original PR:

No backport intended.

cmsbuild · 2019-10-24T16:27:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-24T16:34:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28262/12425

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaElectronAlgos/interface/PixelHitMatcher.h modified in PR(s): Fix problem with fromTrackerSeeds parameter in ElectronSeedProducer and improve LayerMeasurements class #28039
- File RecoEgamma/EgammaElectronAlgos/src/PixelHitMatcher.cc modified in PR(s): Fix problem with fromTrackerSeeds parameter in ElectronSeedProducer and improve LayerMeasurements class #28039

cmsbuild · 2019-10-24T16:34:31Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

RecoEgamma/EgammaElectronAlgos
RecoEgamma/Examples

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @lgray, @sobhatta, @afiqaize, @varuns23 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

slava77 · 2019-10-24T16:39:36Z

@cmsbuild please test

cmsbuild · 2019-10-24T16:40:08Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3142/console Started: 2019/10/24 18:40

cmsbuild · 2019-10-24T17:59:35Z

+1
Tested at: ac6c72d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28ba0e/3142/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

Remove manually generated cfi file for ElectronSeedProducer #28213 @guitargeek: Remove manually generated cfi file for ElectronSeedProducer

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28ba0e/3142/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28ba0e/3142/git-merge-result

cmsbuild · 2019-10-24T17:59:38Z

Comparison job queued.

cmsbuild · 2019-10-24T19:29:25Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2961036
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2960693
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

slava77 · 2019-10-30T21:49:59Z

code-checks

cmsbuild · 2019-10-30T21:50:26Z

The code-checks are being triggered in jenkins.

guitargeek · 2019-11-01T15:13:56Z

That would have worked, but I wanted to reuse the code for the TrajSeedMatcher. So it had to go in some header file because I wanted to inline it for best performance, and I read in the core guidelines that you should not use the anonymous namespace in header files:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-unnamed
And indeed, the compiler would give me a warning if I would use the anonymous namespace there:

RecoEgamma/EgammaElectronAlgos/interface/TrajSeedMatcher.h:48:7: warning: 'TrajSeedMatcher' has a field 'TrajSeedMatcher::trajStateFromPointPosChargeCache_' whose type uses the anonymous namespace [-Wsubobject-linkage]

slava77 · 2019-11-01T15:35:06Z

RecoEgamma/EgammaElectronAlgos/src/PixelHitMatcher.cc

@@ -173,7 +217,7 @@ std::vector<SeedWithInfo> PixelHitMatcher::operator()(const std::vector<const Tr
          auto idx2 = std::distance(hits.first, it2);
          const DetId id2 = it2->geographicalId();
          const GeomDet *geomdet2 = it2->det();
-          const std::pair<const GeomDet *, GlobalPoint> det_key(geomdet2, hit1Pos);
+          const auto det_key = std::make_pair(geomdet2->gdetIndex(), hit1Pos);


the logic of the map filling is different now.
Is the gdetIndex unique?

I honestly didn't really check, I just did this for the sake of unifying with the HLT code which does that. I trust Sams modification there. It's certainly nicer than hashing the memory location which might cause bugs if you at some place copied the GeomDet!

slava77 · 2019-11-01T15:35:30Z

That would have worked, but I wanted to reuse the code for the TrajSeedMatcher

I see, a feature creep

slava77 · 2019-11-01T16:05:33Z

On 11/1/19 9:01 AM, Slava Krutelyov wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In RecoEgamma/EgammaElectronAlgos/interface/TrajSeedMatcher.h > @@ -290,8 +272,8 @@ class TrajSeedMatcher { std::unordered_map<int, TrajectoryStateOnSurface> trajStateFromVtxPosChargeCache_; std::unordered_map<int, TrajectoryStateOnSurface> trajStateFromVtxNegChargeCache_; change these too?

nm, wrong type => not applicable

…

slava77 · 2019-11-01T21:38:43Z

@cmsbuild please test

cmsbuild · 2019-11-01T21:39:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3310/console Started: 2019/11/01 22:41

cmsbuild · 2019-11-01T23:07:50Z

+1
Tested at: 50c5ef8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28ba0e/3310/summary.html

cmsbuild · 2019-11-01T23:07:53Z

Comparison job queued.

cmsbuild · 2019-11-02T00:41:39Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2939026
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2938683
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

slava77 · 2019-11-04T18:03:21Z

+1

for #28262 50c5ef8

essentially all technical code changes are in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2019-11-04T18:03:52Z

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 · 2019-11-04T21:09:59Z

+1

smuzaffar · 2019-11-06T11:15:53Z

@slava77 , @fabiocos ( referring to your cmment: #28262 (comment) ) we had missed few github web hooks that is why for some PRs the https://github.com/cms-sw/cms-prs/blob/master/cms-sw/cmssw/.other/files_changed_by_prs.json was not updated. This has been fixed now and just to make sure that we have not missed any thing we force update it every 4 hours.

guitargeek added 2 commits October 24, 2019 18:05

slim down the measurement estimators

3892199

Merged the MeasurementEstimators into the PixelHitMatcher

ac6c72d

cmsbuild added this to the CMSSW_11_0_X milestone Oct 24, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 24, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 24, 2019

cmsbuild added tests-started and removed tests-pending labels Oct 24, 2019

cmsbuild added tests-approved and removed tests-started labels Oct 24, 2019

cmsbuild added comparison-available and removed comparison-pending labels Oct 24, 2019

guitargeek mentioned this pull request Oct 25, 2019

Many classes in EgammaElectronAlgos turned into pure functions #28282

Merged

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 30, 2019

slava77 reviewed Nov 1, 2019

View reviewed changes

cms-sw deleted a comment from guitargeek Nov 1, 2019

cmsbuild added tests-started and removed tests-pending labels Nov 1, 2019

cmsbuild added tests-approved and removed tests-started labels Nov 1, 2019

cmsbuild added comparison-available and removed comparison-pending labels Nov 2, 2019

fabiocos mentioned this pull request Nov 2, 2019

Incorrect list of open PRs for overlap check cms-sw/cms-bot#1221

Closed

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Nov 4, 2019

cmsbuild added orp-approved and removed orp-pending labels Nov 4, 2019

cmsbuild merged commit 2e30930 into cms-sw:master Nov 4, 2019

cmsbuild mentioned this pull request Nov 5, 2019

Apply code formatting to FWCore/Framework #28345

Merged

guitargeek deleted the PixelHitMatcher_MeasurementEstimators branch November 5, 2019 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merged the MeasurementEstimators into the PixelHitMatcher #28262

Merged the MeasurementEstimators into the PixelHitMatcher #28262

guitargeek commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

slava77 commented Oct 24, 2019

cmsbuild commented Oct 24, 2019 •

edited

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

slava77 commented Oct 30, 2019

cmsbuild commented Oct 30, 2019

guitargeek commented Nov 1, 2019 •

edited

slava77 Nov 1, 2019

guitargeek Nov 1, 2019 •

edited

slava77 commented Nov 1, 2019

slava77 commented Nov 1, 2019 via email

slava77 commented Nov 1, 2019

cmsbuild commented Nov 1, 2019 •

edited

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 2, 2019

slava77 commented Nov 4, 2019

cmsbuild commented Nov 4, 2019

fabiocos commented Nov 4, 2019

smuzaffar commented Nov 6, 2019

Merged the MeasurementEstimators into the PixelHitMatcher #28262

Merged the MeasurementEstimators into the PixelHitMatcher #28262

Conversation

guitargeek commented Oct 24, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

slava77 commented Oct 24, 2019

cmsbuild commented Oct 24, 2019 • edited

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

cmsbuild commented Oct 24, 2019

slava77 commented Oct 30, 2019

cmsbuild commented Oct 30, 2019

guitargeek commented Nov 1, 2019 • edited

slava77 Nov 1, 2019

Choose a reason for hiding this comment

guitargeek Nov 1, 2019 • edited

Choose a reason for hiding this comment

slava77 commented Nov 1, 2019

slava77 commented Nov 1, 2019 via email

slava77 commented Nov 1, 2019

cmsbuild commented Nov 1, 2019 • edited

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 2, 2019

slava77 commented Nov 4, 2019

cmsbuild commented Nov 4, 2019

fabiocos commented Nov 4, 2019

smuzaffar commented Nov 6, 2019

cmsbuild commented Oct 24, 2019 •

edited

guitargeek commented Nov 1, 2019 •

edited

guitargeek Nov 1, 2019 •

edited

cmsbuild commented Nov 1, 2019 •

edited