Update & fix to SeedingHitSet #36819

AdrianoDee · 2022-01-27T14:25:19Z

PR description:

Updating SeedingHitSet:

filtering out nullptr directly in the constructors dropping setSize();
adding std::initializer_list to allow braced constructors, to have a cleaner doublet, triplet and quadruplet hit constructors;
relying on std::size for the size (with the correction of single-hit set);
adding tests in SeedingHitSet_t.cpp;
fix bugs in segfaults in CAHitQuadrupletEDProducer #36808 & Segfault in HitPairEDProducer #36804

PR validation:

The tests run.
Tested wfs 1030.0 and 1040.0with CMSSW_12_3_ASAN_X_2022-01-26-2300 (that was highlighting the failures) + this PR and it's running.

FYI: @makortel, @dan131riley, @Dr15Jones

cmsbuild · 2022-01-27T14:36:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36819/27978

This PR adds an extra 12KB to repository

cmsbuild · 2022-01-27T14:36:48Z

A new Pull Request was created by @AdrianoDee for master.

It involves the following packages:

RecoTracker/TkSeedingLayers (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

AdrianoDee · 2022-01-27T14:39:32Z

test parameters:

workflows = 1030.0,1040.0
enable_test = threading,profiling

AdrianoDee · 2022-01-27T14:40:09Z

please test

AdrianoDee · 2022-01-27T14:46:52Z

type bug-fix

Dr15Jones · 2022-01-27T14:50:30Z

RecoTracker/TkSeedingLayers/test/SeedingHitSet_t.cpp

@@ -0,0 +1,80 @@
+#include "RecoTracker/TkSeedingLayers/interface/SeedingHitSet.h"


Great to see a unit test! Please consider using catch2 system as it provides a richer set of testing abilities: https://github.com/catchorg/Catch2

We make use of Catch2 throughout the framework for our unit tests.

Ah! I see, thanks. Sure, let me have a look at it to restructure the test.

I've mimicked some other tests around the framework. Should be fine.

makortel · 2022-01-27T16:38:50Z

RecoTracker/TkSeedingLayers/interface/SeedingHitSet.h

+    std::copy_n(
+        hits.begin(), std::find(hits.begin(), hits.end(), nullPtr()) - hits.begin(), std::back_inserter(theRecHits));


Could this be implemented also

Suggested change

std::copy_n(

hits.begin(), std::find(hits.begin(), hits.end(), nullPtr()) - hits.begin(), std::back_inserter(theRecHits));

std::copy(

hits.begin(), std::find(hits.begin(), hits.end(), nullPtr()), std::back_inserter(theRecHits));

? (genuinely asking, I'd think so)

Yes, actually it could: moving to it together with the suggestion below.

makortel · 2022-01-27T16:47:47Z

RecoTracker/TkSeedingLayers/interface/SeedingHitSet.h

+    theRecHits.reserve(hits.size());
+    std::copy_n(
+        hits.begin(), std::find(hits.begin(), hits.end(), nullPtr()) - hits.begin(), std::back_inserter(theRecHits));
+    theSize = theRecHits.size() > 1 ? theRecHits.size() : 0;


How about

Suggested change

theSize = theRecHits.size() > 1 ? theRecHits.size() : 0;

if (theRecHits.size() == 1) theRecHits.clear()

and in size() return theRecHits.size()?

Or something along

auto end = std::find(hits.begin(), hits.end(), nullPtr()); auto size = std::distance(hits.begin(), end); if (size >= 2) { theRecHits.reserve(size); std::copy(hits.begin, end, std::back_inserter(theRecHits)); }

?

cmsbuild · 2022-01-28T07:13:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afa21f/22048/summary.html
COMMIT: 7299be6
CMSSW: CMSSW_12_3_ASAN_X_2022-01-26-2300/slc7_amd64_gcc11
Additional Tests: THREADING,PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36819/22048/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1030.0_RunHLTPhy2017B+RunHLTPhy2017B+TIER0EXPHPBS+ALCASPLITHPBS+ALCAHARVDHPBS+ALCAHARVDHPBSLOWPU
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1040.0_RunZeroBias2017F+RunZeroBias2017F+TIER0RAWSIPIXELCAL+ALCASPLITSIPIXELCAL+ALCAHARVDSIPIXELCAL

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
Reco comparison had 73 failed jobs
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3449324
DQMHistoTests: Total failures: 1049328
DQMHistoTests: Total nulls: 65
DQMHistoTests: Total successes: 2399909
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.186 KiB( 42 files compared)
DQMHistoSizes: changed ( 10224.0 ): -0.117 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 11834.0 ): -0.352 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 250202.181 ): 0.533 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 25202.0 ): -0.246 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 181 log files, 42 edm output root files, 43 DQM output files

clacaputo · 2022-01-28T08:47:34Z

@smuzaffar in the test there are 73 RECO comparisons jobs failing with this error

==23339==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

Is it something that we should expect from ASAN builds?

smuzaffar · 2022-01-28T08:57:25Z

please test
@clacaputo , comparison jobs are not working for ASAN IBs (I am thinking to disable the comparison for ASAN PR tests). The ASAN tests show that failing workflows are now running fine. I have started the tests for normal IBs now

cmsbuild · 2022-01-28T15:28:13Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afa21f/22056/summary.html
COMMIT: 7299be6
CMSSW: CMSSW_12_3_X_2022-01-27-2300/slc7_amd64_gcc10
Additional Tests: THREADING,PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36819/22056/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1030.0_RunHLTPhy2017B+RunHLTPhy2017B+TIER0EXPHPBS+ALCASPLITHPBS+ALCAHARVDHPBS+ALCAHARVDHPBSLOWPU
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1040.0_RunZeroBias2017F+RunZeroBias2017F+TIER0RAWSIPIXELCAL+ALCASPLITSIPIXELCAL+ALCAHARVDSIPIXELCAL

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3449612
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3449590
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

cmsbuild · 2022-01-31T14:12:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36819/28036

This PR adds an extra 12KB to repository
Found files with invalid states:
- RecoTracker/TkSeedingLayers/test/SeedingHitSet_t.cpp:
  - Added: 22d115d
  - Deleted: d0ac3c6

cmsbuild · 2022-01-31T14:12:38Z

Pull request #36819 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

AdrianoDee · 2022-01-31T14:56:57Z

please test

cmsbuild · 2022-01-31T21:03:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afa21f/22102/summary.html
COMMIT: d0ac3c6
CMSSW: CMSSW_12_3_X_2022-01-30-2300/slc7_amd64_gcc10
Additional Tests: THREADING,PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36819/22102/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1030.0_RunHLTPhy2017B+RunHLTPhy2017B+TIER0EXPHPBS+ALCASPLITHPBS+ALCAHARVDHPBS+ALCAHARVDHPBSLOWPU
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-afa21f/1040.0_RunZeroBias2017F+RunZeroBias2017F+TIER0RAWSIPIXELCAL+ALCASPLITSIPIXELCAL+ALCAHARVDSIPIXELCAL

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3449612
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3449584
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

clacaputo · 2022-02-01T11:59:45Z

+reconstruction

fix bugs in segfaults in CAHitQuadrupletEDProducer segfaults in CAHitQuadrupletEDProducer #36808 & Segfault in HitPairEDProducer Segfault in HitPairEDProducer #36804
wf 1030.0, 1040.0 tested successfully in ASAN build Update & fix to SeedingHitSet #36819 (comment)
new unit test added

cmsbuild · 2022-02-01T12:00:07Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

mmusich · 2022-02-01T15:54:49Z

ping @cms-sw/orp-l2 it would be great to have this in, in order to test #36852 cleanly.

perrotta · 2022-02-01T17:56:43Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Jan 27, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 27, 2022

AdrianoDee force-pushed the seeding_hits_set_fix branch from fbe7af2 to 7299be6 Compare January 27, 2022 14:28

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 27, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 27, 2022

AdrianoDee mentioned this pull request Jan 27, 2022

Segfault in HitPairEDProducer #36804

Closed

cmsbuild added the bug-fix label Jan 27, 2022

Dr15Jones reviewed Jan 27, 2022

View reviewed changes

makortel reviewed Jan 27, 2022

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jan 28, 2022

cmsbuild added tests-started and removed tests-approved labels Jan 28, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 28, 2022

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved labels Jan 31, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 31, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 31, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 31, 2022

cmsbuild added fully-signed and removed reconstruction-pending pending-signatures labels Feb 1, 2022

cmsbuild added the reconstruction-approved label Feb 1, 2022

mmusich mentioned this pull request Feb 1, 2022

Fix SiStripCluster::size return type #36669

Closed

mmusich mentioned this pull request Feb 1, 2022

Change to SiStripCluster DataFormat: use auto for return types #36852

Merged

makortel mentioned this pull request Feb 1, 2022

Hydjet++ 2.4.3 integration #36316

Merged

cmsbuild added orp-approved and removed orp-pending labels Feb 1, 2022

cmsbuild merged commit 3e98b1b into cms-sw:master Feb 1, 2022

cmsbuild mentioned this pull request Feb 2, 2022

Clean DQMOffline/MuonDPG/BuildFile.xml and remove UBSAN comments #36855

Merged

mmusich mentioned this pull request Feb 15, 2022

segfaults in CAHitQuadrupletEDProducer #36808

Closed

AdrianoDee deleted the seeding_hits_set_fix branch February 26, 2022 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update & fix to SeedingHitSet #36819

Update & fix to SeedingHitSet #36819

AdrianoDee commented Jan 27, 2022 •

edited

Loading

cmsbuild commented Jan 27, 2022

cmsbuild commented Jan 27, 2022

AdrianoDee commented Jan 27, 2022 •

edited by smuzaffar

Loading

AdrianoDee commented Jan 27, 2022

AdrianoDee commented Jan 27, 2022

Dr15Jones Jan 27, 2022

AdrianoDee Jan 27, 2022

AdrianoDee Jan 28, 2022

makortel Jan 27, 2022

AdrianoDee Jan 28, 2022

makortel Jan 27, 2022

cmsbuild commented Jan 28, 2022

clacaputo commented Jan 28, 2022

smuzaffar commented Jan 28, 2022

cmsbuild commented Jan 28, 2022

cmsbuild commented Jan 31, 2022

cmsbuild commented Jan 31, 2022

AdrianoDee commented Jan 31, 2022

cmsbuild commented Jan 31, 2022

clacaputo commented Feb 1, 2022

cmsbuild commented Feb 1, 2022

mmusich commented Feb 1, 2022

perrotta commented Feb 1, 2022

		@@ -0,0 +1,80 @@
		#include "RecoTracker/TkSeedingLayers/interface/SeedingHitSet.h"

		std::copy_n(
		hits.begin(), std::find(hits.begin(), hits.end(), nullPtr()) - hits.begin(), std::back_inserter(theRecHits));

	theSize = theRecHits.size() > 1 ? theRecHits.size() : 0;
	if (theRecHits.size() == 1) theRecHits.clear()

Update & fix to SeedingHitSet #36819

Update & fix to SeedingHitSet #36819

Conversation

AdrianoDee commented Jan 27, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Jan 27, 2022

cmsbuild commented Jan 27, 2022

AdrianoDee commented Jan 27, 2022 • edited by smuzaffar Loading

AdrianoDee commented Jan 27, 2022

AdrianoDee commented Jan 27, 2022

Dr15Jones Jan 27, 2022

Choose a reason for hiding this comment

AdrianoDee Jan 27, 2022

Choose a reason for hiding this comment

AdrianoDee Jan 28, 2022

Choose a reason for hiding this comment

makortel Jan 27, 2022

Choose a reason for hiding this comment

AdrianoDee Jan 28, 2022

Choose a reason for hiding this comment

makortel Jan 27, 2022

Choose a reason for hiding this comment

cmsbuild commented Jan 28, 2022

Comparison Summary

clacaputo commented Jan 28, 2022

smuzaffar commented Jan 28, 2022

cmsbuild commented Jan 28, 2022

Comparison Summary

cmsbuild commented Jan 31, 2022

cmsbuild commented Jan 31, 2022

AdrianoDee commented Jan 31, 2022

cmsbuild commented Jan 31, 2022

Comparison Summary

clacaputo commented Feb 1, 2022

cmsbuild commented Feb 1, 2022

mmusich commented Feb 1, 2022

perrotta commented Feb 1, 2022

AdrianoDee commented Jan 27, 2022 •

edited

Loading

AdrianoDee commented Jan 27, 2022 •

edited by smuzaffar

Loading