Simulation: define PSimHit track Id offset and propagate to MTD hit categories #42506

fabiocos · 2023-08-08T09:32:20Z

PR description:

Following the discussion about #42455 at the simulation meeting https://indico.cern.ch/event/1313529/ , this PR implements:

a default base track id offset in the PSimHit class, and methods to add and retrieve offsets and the original track id;
propagates its use to the MtdSD class, keeping the previous logical behaviour unaltered;
adds a protection in the SimTrackManager throwing an exception if tracks with a track Id larger than the base offset are proposed for permanent storage.

The previous offset of 100000000 is doubled to 200000000 , in order to stay safe also in presence of Heavy Ion samples. A test with 1k events of wf. 24950 shows that track Ids close to 90k are possible in these very populated events. The chosen base offset allows anyway a large number of possible offsets to be stored, before hitting the limit of maximum size of an unsigned integer (UINT_MAX = 4294967295).

@makortel The proposed implementation does not technically modify persistent data members or existing interfaces, and for this reason no change of class version for schema evolution is added. The track Id member meaning is potentially enlarged now, but in a way that should be backward compatible (although no mechanism prevents in principle track ids larger than the base offset for the past, they are not expected for the reasons mentioned above).

PR validation:

Code runs, and provides the expected track ids in BTL hits, as shown by the dump of the test class SimHitCaloHitDumper updated in this PR. e.g.

BTLHits hits in the event = 160


1653680716  (2.65248,0.0823182,0.866362)  4.77149 Energy = 0.00187623 Track Id = 104
1653623364  (-23.5429,0.117936,1.5)  5.13729 Energy = 0.00161607 Track Id = 104
1653623365  (-24.0755,-1.56,0.419155)  5.14377 Energy = 0.00234347 Track Id = 104
1653623365  (-24.306,-0.0968286,-1.34526)  5.15137 Energy = 0.000306122 Track Id = 104
1654395712  (-19.9635,-0.910802,0.345843)  12.8854 Energy = 0.000179942 Track Id = 400000104
1654392652  (-12.1859,-1.46623,1.02566)  12.7973 Energy = 0.000569917 Track Id = 400000104
1653999172  (13.007,1.17016,0.636569)  10.2406 Energy = 0.000445038 Track Id = 400000104
1653672524  (-4.91178,-0.848405,1.26667)  7.25785 Energy = 7.98462e-05 Track Id = 600000104
1653680719  (-9.67319,0.640687,1.5)  4.83673 Energy = 0.00299444 Track Id = 104
1653683776  (-10.1171,-1.56,0.396154)  4.84311 Energy = 0.0044181 Track Id = 104
1653680719  (-11.8098,0.169878,1.29515)  4.84187 Energy = 4.56779e-05 Track Id = 104
1658860419  (-11.4223,1.38335,-1.1103)  14.1081 Energy = 0.000175683 Track Id = 200000097
1658465159  (-1.0942,-1.36393,-0.899755)  15.5332 Energy = 0.000232008 Track Id = 600000097
1657874062  (5.81779,-0.420891,0.367464)  17.0341 Energy = 9.31307e-06 Track Id = 600000095
1657938761  (-20.3771,-0.024968,-1.10717)  16.9043 Energy = 0.000180497 Track Id = 600000095
1658477452  (20.7793,-0.806021,-1.09933)  21.7767 Energy = 8.5945e-05 Track Id = 600000095
1658855297  (18.1346,-0.17453,-1.09467)  19.5952 Energy = 0.000181718 Track Id = 600000095
1658470222  (-26.2811,-0.0460031,-1.09773)  20.8468 Energy = 9.98849e-05 Track Id = 600000095
1656816974  (1.77915,1.51598,-1.5887)  27.6375 Energy = 0.000116488 Track Id = 600000095
1657162311  (10.6521,-0.669832,-0.934452)  23.207 Energy = 7.398e-05 Track Id = 600000094
1657414280  (19.3899,-1.10402,-1.37687)  21.04 Energy = 0.000219651 Track Id = 600000094
1659064132  (16.9131,0.625088,0.0171433)  11.4986 Energy = 0.000175298 Track Id = 200000093
1654934147  (-11.4032,0.155602,-1.45387)  24.4696 Energy = 6.67964e-05 Track Id = 600000091
1654916993  (5.30227,-1.55327,-0.816225)  16.2435 Energy = 0.000136397 Track Id = 600000091
1654147467  (-8.32702,1.05475,-1.77296)  19.5695 Energy = 0.000112557 Track Id = 600000091
1654408013  (26.8781,-0.0967704,0.240067)  17.055 Energy = 6.3314e-05 Track Id = 600000091
1654408013  (26.8781,-0.0967704,0.240067)  17.055 Energy = 0.00026036 Track Id = 600000091
1654460047  (-23.8675,1.4109,-0.816348)  21.194 Energy = 0.000118696 Track Id = 600000091
1654808458  (17.9005,1.04276,-1.19358)  13.5182 Energy = 0.000903361 Track Id = 600000090
1659050564  (8.15412,-0.0136119,1.5)  4.77013 Energy = 0.0021346 Track Id = 86
1659050565  (7.92549,-1.56,0.25996)  4.77723 Energy = 0.00107187 Track Id = 86
(...)

…ion for track Ids exceeding it

cmsbuild · 2023-08-08T09:38:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42506/36513

This PR adds an extra 24KB to repository

cmsbuild · 2023-08-08T09:38:29Z

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

SimDataFormats/TrackingHit (simulation)
SimG4CMS/Forward (simulation)
SimG4Core/Application (simulation)
SimG4Core/Notification (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@youyingli, @VourMa, @apsallid, @makortel, @JanFSchulte, @rovere, @VinInn, @missirol, @bsunanda, @mmusich, @mtosi, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

fabiocos · 2023-08-08T09:39:48Z

please test

cmsbuild · 2023-08-08T13:21:40Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88909e/34156/summary.html
COMMIT: 7c3287f
CMSSW: CMSSW_13_3_X_2023-08-07-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42506/34156/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 11 lines to the logs
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3150947
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3150925
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

civanch · 2023-08-09T08:15:37Z

+1

offset of track ID is moved to one place without any change of results.

cmsbuild · 2023-08-09T08:16:05Z

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

fabiocos · 2023-08-09T08:19:55Z

hold

cmsbuild · 2023-08-09T08:20:21Z

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

fabiocos · 2023-08-09T08:21:28Z

SimDataFormats/TrackingHit/interface/PSimHit.h

+   */
+  unsigned int originalTrackId() const { return (theTrackId > k_tidOffset) ? theTrackId % k_tidOffset : theTrackId; }
+
+  unsigned int offsetTrackId() const { return (theTrackId > k_tidOffset) ? theTrackId / k_tidOffset : theTrackId; }


@civanch sorry, but I see that there is a logical mistake here: in case no offset is added, this method should be logically return 0, not the trackid, do you agree? I will fix it, as the method is not used anywhere so far the impact of this is invisible in tests

Yes, you are right, I overlook this. If you fix this, then there is a question: may be originalTrackId() is not needed, we may simply move this implementation to the method trackId()

perrotta · 2023-08-09T09:13:00Z

SimDataFormats/TrackingHit/interface/PSimHit.h

+  /** In case te SimTrack ID is incremented by the k_tidOffset for hit category definition, this
+   * methods returns the original theTrackId value directly.
+   */
+  unsigned int originalTrackId() const { return (theTrackId > k_tidOffset) ? theTrackId % k_tidOffset : theTrackId; }


@fabiocos since you are fixing anyhow, I think this could become more simply

Suggested change

unsigned int originalTrackId() const { return (theTrackId > k_tidOffset) ? theTrackId % k_tidOffset : theTrackId; }

unsigned int originalTrackId() const { return theTrackId % k_tidOffset; }

@perrotta yes indeed, I have added this in my branch, and I also extend the dumper to test these methods directly...

cmsbuild · 2023-08-09T09:42:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42506/36532

This PR adds an extra 24KB to repository

cmsbuild · 2023-08-09T09:43:19Z

Pull request #42506 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

fabiocos · 2023-08-09T09:44:20Z

please test

cmsbuild · 2023-08-09T15:49:45Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88909e/34185/summary.html
COMMIT: 49cd3e2
CMSSW: CMSSW_13_3_X_2023-08-08-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42506/34185/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

You potentially added 15 lines to the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3150947
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3150919
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

fabiocos · 2023-08-10T08:26:24Z

please test

cmsbuild · 2023-08-10T12:10:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88909e/34207/summary.html
COMMIT: 49cd3e2
CMSSW: CMSSW_13_3_X_2023-08-09-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42506/34207/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 200 lines to the logs
Reco comparison results: 49 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3150947
DQMHistoTests: Total failures: 281
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3150644
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

fabiocos · 2023-08-10T12:55:37Z

Changes appear unrelated to this PR, there are a lot of complaints from tensorflow in the log-files.

civanch · 2023-08-10T13:07:39Z

+1

cmsbuild · 2023-08-10T13:08:04Z

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

makortel · 2023-08-10T13:34:38Z

Changes appear unrelated to this PR, there are a lot of complaints from tensorflow in the log-files.

Looks like #42525

perrotta · 2023-08-10T15:31:13Z

+1

Define PSimHit offset and propagate to MTD hit categories, add except…

7c3287f

…ion for track Ids exceeding it

cmsbuild added this to the CMSSW_13_3_X milestone Aug 8, 2023

cmsbuild added simulation-pending pending-signatures tests-pending orp-pending code-checks-pending labels Aug 8, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 8, 2023

cmsbuild added tests-started and removed tests-pending labels Aug 8, 2023

cmsbuild added tests-approved and removed tests-started labels Aug 8, 2023

cmsbuild added simulation-approved fully-signed and removed simulation-pending pending-signatures labels Aug 9, 2023

cmsbuild added pending-signatures hold and removed fully-signed labels Aug 9, 2023

fabiocos commented Aug 9, 2023

View reviewed changes

perrotta reviewed Aug 9, 2023

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 9, 2023

cmsbuild added tests-started and removed tests-pending labels Aug 9, 2023

cmsbuild added tests-rejected and removed tests-started labels Aug 9, 2023

cmsbuild added tests-started and removed tests-rejected labels Aug 10, 2023

cmsbuild added tests-approved and removed tests-started labels Aug 10, 2023

cmsbuild added simulation-approved fully-signed and removed simulation-pending pending-signatures labels Aug 10, 2023

makortel mentioned this pull request Aug 10, 2023

Spurious DQM differences in MVA-related quantities (tensorflow related ?) #42525

Open

cmsbuild added orp-approved and removed orp-pending labels Aug 10, 2023

cmsbuild merged commit 6975683 into cms-sw:master Aug 10, 2023

fabiocos deleted the fc-psimhitOffset branch August 11, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simulation: define PSimHit track Id offset and propagate to MTD hit categories #42506

Simulation: define PSimHit track Id offset and propagate to MTD hit categories #42506

fabiocos commented Aug 8, 2023 •

edited

Loading

cmsbuild commented Aug 8, 2023

cmsbuild commented Aug 8, 2023

fabiocos commented Aug 8, 2023

cmsbuild commented Aug 8, 2023

civanch commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos Aug 9, 2023 •

edited

Loading

civanch Aug 9, 2023

perrotta Aug 9, 2023

fabiocos Aug 9, 2023

cmsbuild commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos commented Aug 10, 2023

cmsbuild commented Aug 10, 2023

fabiocos commented Aug 10, 2023

civanch commented Aug 10, 2023

cmsbuild commented Aug 10, 2023

makortel commented Aug 10, 2023

perrotta commented Aug 10, 2023

	unsigned int originalTrackId() const { return (theTrackId > k_tidOffset) ? theTrackId % k_tidOffset : theTrackId; }
	unsigned int originalTrackId() const { return theTrackId % k_tidOffset; }

Simulation: define PSimHit track Id offset and propagate to MTD hit categories #42506

Simulation: define PSimHit track Id offset and propagate to MTD hit categories #42506

Conversation

fabiocos commented Aug 8, 2023 • edited Loading

PR description:

PR validation:

cmsbuild commented Aug 8, 2023

cmsbuild commented Aug 8, 2023

fabiocos commented Aug 8, 2023

cmsbuild commented Aug 8, 2023

Comparison Summary

civanch commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

civanch Aug 9, 2023

Choose a reason for hiding this comment

perrotta Aug 9, 2023

Choose a reason for hiding this comment

fabiocos Aug 9, 2023

Choose a reason for hiding this comment

cmsbuild commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

fabiocos commented Aug 9, 2023

cmsbuild commented Aug 9, 2023

RelVals-INPUT

Comparison Summary

fabiocos commented Aug 10, 2023

cmsbuild commented Aug 10, 2023

Comparison Summary

fabiocos commented Aug 10, 2023

civanch commented Aug 10, 2023

cmsbuild commented Aug 10, 2023

makortel commented Aug 10, 2023

perrotta commented Aug 10, 2023

fabiocos commented Aug 8, 2023 •

edited

Loading

fabiocos Aug 9, 2023 •

edited

Loading