-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New version of RawPrime (SiStripApproximateCluster) #49013
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
Conversation
|
@cmsbuild ping |
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49013/46205
|
|
A new Pull Request was created by @silviodonato for master. It involves the following packages:
@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild, please test |
|
Currently something is missing here. Difference of detId are not considered and the variable beginIndices are still present in the collection. The correspnding codes DataFormats/SiStripCluster/interface/SiStripApproximateClusterCollection_v1.h, DataFormats/SiStripCluster/src/SiStripApproximateClusterCollection_v1.cc are deleted. |
|
-1 Failed Tests: UnitTests RelVals Unit TestsI found 1 errors in the following unit tests: ---> test TestDQMOnlineClient-sistrip_approx_dqm_sourceclient had ERRORS RelVals |
…inor improvements in readibility
f465968 to
89eef23
Compare
| return barycenter_ * 0.1; // in the old format barycenter_ is in tenths of strips | ||
| case 2: { | ||
| // Drop the first bit (encoding the saturation info) | ||
| double barycenter_decoded = (barycenter_ & 0b0111'1111'1111'1111); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't those bitmasks be a constant somewhere, so it's more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I've changed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! :)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49013/46216
|
|
Pull request #49013 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again. |
|
please test with cms-data/DQM-Integration#13 |
|
-1 Failed Tests: RelVals RelVals
|
unrelated failures, see #49018. The next IB should be clean (at lease for the non-DAS related ones) |
|
please test with cms-data/DQM-Integration#13 |
|
+1 Size: This PR adds an extra 16KB to repository The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
Closed in favor of #49015 |
PR description:
This PR adds an improved version of RawPrime (SiStripApproximateCluster).
The new version has been developed by @saswatinandan , the results are described in CMS DP-2025/031, EPS talk, NGT presentation (see previous PR).
On top of this, I applied some changes to allow CMSSW to run both versions of RawPrime using the same code (at the cost of adding a
version_flag in theSiStripApproximateClusterdataformat) + some other minor improvements, mainly about code readability.In summary:
version=1is used and it is expect to give no changes at allversion=2:avgChargeis stored using 6 bits instead of 8 (ie. with a precision of 255/64 = 4 ADC)filter_andpeakFilter_flagsbarycenter_will save the distance between the cluster barycenter instead of the cluster barycenter positionisSaturated_flag is encoded in the firstbarycenter_bitgetBarycenter(...)andgetAvgCharge(...)have been added to returnfloatinstead ofintamplitudes_of aSiStripClusterhave been improved to give exactly the same value asSiStripCluster::charge().detId_instead of their value.Note: The datamember
isSaturated_,peakFilter_andfilter_are not used anymore in V2 and should be removed when V1 will not be used anymore.PR validation:
I prepared a ConfDb area for testing
/users/sdonato/2025Sept/RawSecond/HLT/V11which creates two SiStripApproxClusters collectionshltSiStripClusters2ApproxClustersandhltSiStripClusters2ApproxClustersV2.It also decompresses these collections to standard si strip clusters
hltSiStripClustersFromRawPrimeandhltSiStripClustersFromRawPrimeV2.You can download the configuration from
hltGetConfiguration /users/sdonato/2025Sept/RawSecond/HLT/V11 --globaltag auto:run3_hlt_HIon --input file:...aRAWfile.root --max-events 100 --eras Run3_2025 --prescale none --l1-emulator uGT --l1 L1Menu_Collisions2025_v1_3_0_xmland then edit the configuration with
to enable the new versions and save the decompressed clusters.
Using this script I made comparisons between clusters collections:
in terms of diff of
Using another customization I computed the data reduction obtained running over 4000 events. It is 8.2% for the full event size (18% considering only siStripApproximateCluster). A further 3.3% of reduction can be achieved applying a cluster charge cut, as shown in the presentation (no PR needed for this, only HLT configuration change).
Test done using LZMA4, run=388350 lumi=11
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
This PR is urgent, as we would like to have a backport to
CMSSW_15_1_Xfor the upcoming HIon collisions. As discussed with TSG (@cms-sw/hlt-l2 ) and HIon (@icali @mandrenguyen) we would like to have a stream online using this new dataformat in the upcoming HIon run for testing (with the idea to use it in 2026 if the test is successful).