update on strip approximate cluster #47367

vmuralee · 2025-02-17T13:23:00Z

PR description:

Correction for Average charge in Approximated cluster

The average charge defined as Int avgCharge = cluster.charge()/cluster.size() which doesn't return to nearest integer value in c++. The affect is shown in average charge difference between RAW and RAW' dataset.
The bug fixes as Int avgCharge - (cluster.charge()+cluster.size()/2)/cluster.size()
Details can found here slide 4

Compression of barycenter

Instead of storing barycenter to 16 bit Int , choose the float variable and compress the variable using std::round(cluster.barycenter() * bit_range / maxBarycenter)
bit_range is the range of each bit value, e.g. bit range for 16 bit is 2^16 - 1 = 65535.
maxBarycenter is the maximum value of barycenter which found that is 770.0.
Details can found here slide 9

Changing the configuration

The Configuration.Eras.Era_Run2024_approxSiStripCluster is the configuration use for run approximate cluster collection in the RECO step.

cmsbuild · 2025-02-17T13:23:26Z

cms-bot internal usage

cmsbuild · 2025-02-17T13:24:02Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43720

ERROR: Unable to merge PR.

See log https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43720/cms-checkout-topic.log

cmsbuild · 2025-02-17T13:37:19Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43722

There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/SiStripApproximateCluster.cc modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/SiStripCluster.cc modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/classes_def.xml modified in PR(s): bug Fix on the average charge computation. #47309

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-checks:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43722/code-checks.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43722/code-checks.patch | patch -p1
You can also run scram build code-checks to apply code checks directly
code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43722/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43722/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2025-02-17T13:41:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43723

There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/SiStripApproximateCluster.cc modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/SiStripCluster.cc modified in PR(s): bug Fix on the average charge computation. #47309
- File DataFormats/SiStripCluster/src/classes_def.xml modified in PR(s): bug Fix on the average charge computation. #47309

cmsbuild · 2025-02-17T13:41:50Z

Pull request #47367 was updated.

mmusich · 2025-02-18T09:44:08Z

Configuration/Eras/python/Era_Run2024_approxSiStripCluster.py

+from Configuration.ProcessModifiers.approxSiStripClusters_cff import approxSiStripClusters
+from Configuration.ProcessModifiers.trackingNoLoopers_cff import trackingNoLoopers
+
+Run3_pp_on_PbPb_approxSiStripClusters_2024 = cms.ModifierChain(Run3_2024.copyAndExclude([trackingNoLoopers]),  approxSiStripClusters) 


Suggested change

Run3_pp_on_PbPb_approxSiStripClusters_2024 = cms.ModifierChain(Run3_2024.copyAndExclude([trackingNoLoopers]), approxSiStripClusters)

Run3_2024_approxSiStripClusters = cms.ModifierChain(Run3_2024.copyAndExclude([trackingNoLoopers]), approxSiStripClusters)

what does this have to do specifically with pp_on_PbPb ?

nothing have to do with pp_on_PbPb, I add the suggestion to the PR.

mmusich · 2025-02-18T09:47:32Z

test parameters:

workflows=159,159.02,159.03,140.56,140.58,140.6

cmsbuild · 2025-02-18T10:46:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43754

Found files with invalid states:
- Configuration/Eras/python/Era_Run2024_approxSiStripCluster.py:
  - Added: de2e3de
  - Modified: bec516a
  - Deleted: 830a7c5

cmsbuild · 2025-02-18T10:46:31Z

Pull request #47367 was updated.

mmusich · 2025-02-18T10:52:57Z

@cmsbuild, please test

mmusich · 2025-02-18T12:51:37Z

@vmuralee this PR is failing TestSiStripApproximateClusterCollection

with:

===== Test "TestSiStripApproximateClusterCollection" ====
Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.028 CET
Begin processing the 2nd record. Run 1, Event 2, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 3rd record. Run 1, Event 3, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 4th record. Run 1, Event 4, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 5th record. Run 1, Event 5, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 6th record. Run 1, Event 6, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 7th record. Run 1, Event 7, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 8th record. Run 1, Event 8, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 9th record. Run 1, Event 9, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
Begin processing the 10th record. Run 1, Event 10, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:26.029 CET
18-Feb-2025 12:43:27 CET  Initiating request to open file file:testSiStripApproximateClusterCollection.root
18-Feb-2025 12:43:27 CET  Successfully opened file file:testSiStripApproximateClusterCollection.root
Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:27.402 CET
Begin processing the 2nd record. Run 1, Event 2, LumiSection 1 on stream 0 at 18-Feb-2025 12:43:27.403 CET
----- Begin Fatal Exception 18-Feb-2025 12:43:27 CET-----------------------
An exception of category 'TestFailure' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 2 stream: 0
   [1] Running path 'path'
   [2] Calling method for module TestReadSiStripApproximateClusterCollection/'testReadSiStripApproximateClusterCollection'
Exception Message:
TestReadSiStripApproximateClusterCollection::analyze, barycenter does not have expected value
----- End Fatal Exception -------------------------------------------------
18-Feb-2025 12:43:27 CET  Closed file file:testSiStripApproximateClusterCollection.root
Failure using test_readSiStripApproximateClusterCollection_cfg.py testSiStripApproximateClusterCollection.root: status 65

---> test TestSiStripApproximateClusterCollection had ERRORS
TestTime:3
^^^^ End Test TestSiStripApproximateClusterCollection ^^^^

see log.

See instructions at https://github.com/cms-sw/cmssw/blob/master/DataFormats/SiStripCluster/README.md to see what needs to be done.

cmsbuild · 2025-02-18T13:51:03Z

-1

Failed Tests: UnitTests
Size: This PR adds an extra 44KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-01ff6f/44461/summary.html
COMMIT: 830a7c5
CMSSW: CMSSW_15_1_X_2025-02-17-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47367/44461/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test TestSiStripApproximateClusterCollection had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 957 differences found in the comparisons
DQMHistoTests: Total files compared: 53
DQMHistoTests: Total histograms compared: 4229690
DQMHistoTests: Total failures: 10993
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4218677
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 52 files compared)
Checked 238 log files, 202 edm output root files, 53 DQM output files
TriggerResults: no differences found

cmsbuild · 2025-02-18T15:37:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47367/43763

Found files with invalid states:
- Configuration/Eras/python/Era_Run2024_approxSiStripCluster.py:
  - Added: de2e3de
  - Modified: bec516a
  - Deleted: 830a7c5
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/StandardSequences/python/Eras.py modified in PR(s): Enable mkFit initialStep for track building in phase-2 era, and introduce noMkFit phase-2 era(s) #47383

cmsbuild · 2025-02-18T15:37:29Z

Pull request #47367 was updated.

mmusich · 2025-02-18T15:39:37Z

DataFormats/SiStripCluster/test/TestReadSiStripApproximateClusterCollection.cc

@@ -77,7 +77,7 @@ namespace edmtest {
      unsigned int j = 0;
      for (const auto& cluster : detClusters) {
        unsigned int iOffset = j + iEvent.id().event();
-        if (cluster.barycenter() != expectedIntegralValues_[1] + iOffset) {
+        if (std::round(cluster.barycenter() * 65535.0 / 770.0) != expectedIntegralValues_[1] + iOffset) {


I don't think this is the way this is intended to be fixed, but rather it should be the input file to be changed according to the the instructions @cms-sw/core-l2 FYI

Right, this test should ensure that both old data and new data are read out correctly. The code here should really compare the return value of cluster.barycenter() to the expected value (that perhaps now should be changed to float?).

The change of meaning of the barycenter_ to compBarycenter_ should be dealt with an IO read rule in the classes_def.xml.

mmusich · 2025-02-19T16:27:20Z

DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h

  cms_uint8_t width_ = 0;
  cms_uint8_t avgCharge_ = 0;
  bool filter_ = false;
  bool isSaturated_ = false;
  bool peakFilter_ = false;
+  static constexpr double maxRange_ = 65535.;  //16 bit
+  static constexpr double maxBarycenter_ = 770.;


why 770 and not 768?

makortel · 2025-02-19T17:43:08Z

DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h

@@ -26,20 +27,26 @@ class SiStripApproximateCluster {
                                     float hitPredPos,
                                     bool peakFilter);

-  cms_uint16_t barycenter() const { return barycenter_; }
+  float barycenter() const {
+    float _barycenter = compBarycenter_ * maxBarycenter_ / maxRange_;


Please avoid underscore as the first character (2.14 in https://cms-sw.github.io/cms_coding_rules.html#2--naming-rules-1)

makortel · 2025-02-19T17:47:32Z

DataFormats/SiStripCluster/src/SiStripApproximateCluster.cc

-  barycenter_ = std::round(cluster.barycenter() * 10);
+  compBarycenter_ = std::round(cluster.barycenter() * maxRange_ / maxBarycenter_);
+  assert(cluster.barycenter() <= maxBarycenter_ && "Got a barycenter > maxBarycenter");
+  assert(compBarycenter_ <= maxRange_ && "Filling compBarycenter > maxRange");


Isn't this condition always true? (maximum possible value of compBaryCenter_ being 2**16-1 = 65535 and maxRange_ being set to 65535.)

update on strip approximate cluster

de2e3de

cmsbuild added this to the CMSSW_15_1_X milestone Feb 17, 2025

cmsbuild added reconstruction-pending operations-pending pending-signatures tests-pending orp-pending code-checks-pending trk changes-dataformats labels Feb 17, 2025

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 17, 2025

fix on classes_def.xml

3196799

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 17, 2025

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 17, 2025

fix on code-check and code-format

2bf3498

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 17, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 17, 2025

vmuralee mentioned this pull request Feb 18, 2025

bug Fix on the average charge computation. #47309

Closed

mmusich reviewed Feb 18, 2025

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 18, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 18, 2025

cmsbuild added tests-started and removed tests-pending labels Feb 18, 2025

cmsbuild mentioned this pull request Feb 18, 2025

Enable mkFit initialStep for track building in phase-2 era, and introduce noMkFit phase-2 era(s) #47383

Merged

cmsbuild added tests-rejected and removed tests-started labels Feb 18, 2025

call the expected barycenter value

c4e8c0c

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Feb 18, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 18, 2025

mmusich reviewed Feb 18, 2025

View reviewed changes

mmusich reviewed Feb 19, 2025

View reviewed changes

makortel reviewed Feb 19, 2025

View reviewed changes

This was referenced Mar 4, 2025

Lowered cluster charge threshold for pixel L1 #47480

Open

Updates for runTheMatrix.py: input checks, GPUs repartition, input recycling #47377

Open

2025 Scenario #47536

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update on strip approximate cluster #47367

update on strip approximate cluster #47367

vmuralee commented Feb 17, 2025

cmsbuild commented Feb 17, 2025 •

edited

Loading

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

mmusich Feb 18, 2025

vmuralee Feb 18, 2025

mmusich commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

mmusich commented Feb 18, 2025

mmusich commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

mmusich Feb 18, 2025

makortel Feb 19, 2025

mmusich Feb 19, 2025

makortel Feb 19, 2025

makortel Feb 19, 2025

	Run3_pp_on_PbPb_approxSiStripClusters_2024 = cms.ModifierChain(Run3_2024.copyAndExclude([trackingNoLoopers]), approxSiStripClusters)
	Run3_2024_approxSiStripClusters = cms.ModifierChain(Run3_2024.copyAndExclude([trackingNoLoopers]), approxSiStripClusters)

update on strip approximate cluster #47367

Are you sure you want to change the base?

update on strip approximate cluster #47367

Conversation

vmuralee commented Feb 17, 2025

PR description:

Correction for Average charge in Approximated cluster

Compression of barycenter

Changing the configuration

cmsbuild commented Feb 17, 2025 • edited Loading

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

cmsbuild commented Feb 17, 2025

mmusich Feb 18, 2025

Choose a reason for hiding this comment

vmuralee Feb 18, 2025

Choose a reason for hiding this comment

mmusich commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

mmusich commented Feb 18, 2025

mmusich commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

Unit Tests

Comparison Summary

cmsbuild commented Feb 18, 2025

cmsbuild commented Feb 18, 2025

mmusich Feb 18, 2025

Choose a reason for hiding this comment

makortel Feb 19, 2025

Choose a reason for hiding this comment

mmusich Feb 19, 2025

Choose a reason for hiding this comment

makortel Feb 19, 2025

Choose a reason for hiding this comment

makortel Feb 19, 2025

Choose a reason for hiding this comment

cmsbuild commented Feb 17, 2025 •

edited

Loading