Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update on strip approximate cluster #47367

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+27 −9
Draft

update on strip approximate cluster #47367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
de2e3de
update on strip approximate cluster
vmuralee Feb 17, 2025
3196799
fix on classes_def.xml
vmuralee Feb 17, 2025
2bf3498
fix on code-check and code-format
vmuralee Feb 17, 2025
bec516a
renamed the config file
vmuralee Feb 18, 2025
830a7c5
approx. cluster config updated
vmuralee Feb 18, 2025
c4e8c0c
call the expected barycenter value
vmuralee Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Configuration/Eras/python/Era_Run3_2024_approxSiStripClusters_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import FWCore.ParameterSet.Config as cms

from Configuration.Eras.Era_Run3_2024_cff import Run3_2024
from Configuration.ProcessModifiers.approxSiStripClusters_cff import approxSiStripClusters

Run3_2024_approxSiStripClusters = cms.ModifierChain(Run3_2024, approxSiStripClusters)
1 change: 1 addition & 0 deletions Configuration/StandardSequences/python/Eras.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def __init__(self):
'Run3_pp_on_PbPb_approxSiStripClusters_2023',
'Run3_pp_on_PbPb_2024',
'Run3_pp_on_PbPb_approxSiStripClusters_2024',
'Run3_2024_approxSiStripClusters',
'Run3_dd4hep',
'Run3_DDD',
'Run3_FastSim',
Expand Down
15 changes: 11 additions & 4 deletions DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,20 @@
#define DataFormats_SiStripCluster_SiStripApproximateCluster_h

#include "FWCore/Utilities/interface/typedefs.h"
#include "assert.h"

class SiStripCluster;
class SiStripApproximateCluster {
public:
SiStripApproximateCluster() {}

explicit SiStripApproximateCluster(cms_uint16_t barycenter,
explicit SiStripApproximateCluster(cms_uint16_t compBarycenter,
cms_uint8_t width,
cms_uint8_t avgCharge,
bool filter,
bool isSaturated,
bool peakFilter = false)
: barycenter_(barycenter),
: compBarycenter_(compBarycenter),
width_(width),
avgCharge_(avgCharge),
filter_(filter),
Expand All @@ -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_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert(_barycenter <= maxBarycenter_ && "Returning barycenter > maxBarycenter");
return _barycenter;
}
cms_uint8_t width() const { return width_; }
cms_uint8_t avgCharge() const { return avgCharge_; }
bool filter() const { return filter_; }
bool isSaturated() const { return isSaturated_; }
bool peakFilter() const { return peakFilter_; }

private:
cms_uint16_t barycenter_ = 0;
cms_uint16_t compBarycenter_ = 0;
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.;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 770 and not 768?

static constexpr double trimMaxADC_ = 30.;
static constexpr double trimMaxFracTotal_ = .15;
static constexpr double trimMaxFracNeigh_ = .25;
Expand Down
7 changes: 5 additions & 2 deletions DataFormats/SiStripCluster/src/SiStripApproximateCluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
#include "DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h"
#include "DataFormats/SiStripCluster/interface/SiStripCluster.h"
#include <algorithm>
#include <cassert>
#include <cmath>

SiStripApproximateCluster::SiStripApproximateCluster(const SiStripCluster& cluster,
unsigned int maxNSat,
float hitPredPos,
bool peakFilter) {
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

width_ = cluster.size();
avgCharge_ = cluster.charge() / cluster.size();
avgCharge_ = (cluster.charge() + cluster.size() / 2) / cluster.size();
filter_ = false;
isSaturated_ = false;
peakFilter_ = peakFilter;
Expand Down
2 changes: 1 addition & 1 deletion DataFormats/SiStripCluster/src/SiStripCluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ SiStripCluster::SiStripCluster(const SiStripDigiRange& range) : firstStrip_(rang
}

SiStripCluster::SiStripCluster(const SiStripApproximateCluster cluster, const uint16_t maxStrips) : error_x(-99999.9) {
barycenter_ = cluster.barycenter() / 10.0;
barycenter_ = cluster.barycenter();
charge_ = cluster.width() * cluster.avgCharge();
amplitudes_.resize(cluster.width(), cluster.avgCharge());
filter_ = cluster.filter();
Expand Down
3 changes: 2 additions & 1 deletion DataFormats/SiStripCluster/src/classes_def.xml
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
<class name="edm::Wrapper<edmNew::DetSetVector<edm::Ref<edmNew::DetSetVector<SiStripCluster>,SiStripCluster,edmNew::DetSetVector<SiStripCluster>::FindForDetSetVector> > >" />


<class name="SiStripApproximateCluster" ClassVersion="6">
<class name="SiStripApproximateCluster" ClassVersion="7">
<version ClassVersion="7" checksum="3059068941"/>
<version ClassVersion="6" checksum="132211472"/>
<version ClassVersion="5" checksum="3495825183"/>
<version ClassVersion="4" checksum="2854791577"/>
Expand Down
2 changes: 1 addition & 1 deletion DataFormats/SiStripCluster/test/TestReadSiStripApproximateClusterCollection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throwWithMessage("barycenter does not have expected value");
}
if (cluster.width() != expectedIntegralValues_[2] + iOffset) {
Expand Down