Add SiStrip ApproximateClusterCollection as a simple format for RAW #42022
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42022/35981
|
|
A new Pull Request was created by @jeongeun (JeongEun Lee) for master. It involves the following packages:
@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild, please test |
makortel
left a comment
makortel
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'd suggest to add some unit tests to ensure the SiStripApproximateClusterCollection functions as expected.
I'd also ask to add similar backwards-compatibility-related tests that we recently added for all other data products that are part of the RAW backwards compatibility guarantee, and add a README.md stating that (see e.g. #41631 for an example).
| * (like all RAW data). Any modifications need to be made with care. | ||
| * Please consult core software group if in doubt. | ||
| **/ | ||
| using namespace std; |
There was a problem hiding this comment.
using namespace is not allowed in the global scope in header files.
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-started
code-checks-approved
labels
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42022/36036 ERROR: Build errors found during clang-tidy run. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42022/36044
|
have you tried re-basing your branch ? 140.58 passes in IBs (log) |
|
If HGCal reconstruction is run in a 2018 HI workflow, there is definitely something that does not work in (your implementation) of the otherwise running workflow. |
I don't see how this is possible. This PR is not touching any configuration fragment. |
|
I redo with git cms-rebase-topic jeongeun:ApproxCluster_dataformat now. (unfortunately it's very slow in my lxplus.. I will check and update as soon as I can) |
|
@mmusich @mandrenguyen @perrotta And as suggested, I've fixed DataFormats/L1TParticleFlow/src/classes_def.xml file and run I've just finished Currently, I'm running scram b again. |
|
@jeongeun please start from the PR as it is. |
|
Hi @jeongeun The branch thusly prepared compiles (but fails running 140.58) |
@jeongeun do whatever you think it is quicker. |
I followed this recipe and checked with 140.58 first. error In 140.58_RunHI2018/step3_RunHI2018.log error message coming from SiStripMonitorApproximateCluster/plugins/SiStripMonitorApproximateCluster.cc |
yes, that's what I was writing above (#42022 (comment)), but afaik that should come from the changes as in this PR. |
I think this change is needed diff --git a/DQM/SiStripMonitorApproximateCluster/plugins/SiStripMonitorApproximateCluster.cc b/DQM/SiStripMonitorApproximateCluster/plugins/SiStripMonitorApproximateCluster.cc
index 7c0c8d5167c..b497c4513c8 100644
--- a/DQM/SiStripMonitorApproximateCluster/plugins/SiStripMonitorApproximateCluster.cc
+++ b/DQM/SiStripMonitorApproximateCluster/plugins/SiStripMonitorApproximateCluster.cc
@@ -21,6 +21,7 @@
#include "DQMServices/Core/interface/MonitorElement.h"
#include "DataFormats/Common/interface/DetSet.h"
#include "DataFormats/Common/interface/DetSetVectorNew.h"
+#include "DataFormats/SiStripCluster/interface/SiStripApproximateClusterCollection.h"
#include "DataFormats/SiStripCluster/interface/SiStripApproximateCluster.h"
#include "DataFormats/SiStripCluster/interface/SiStripCluster.h"
#include "FWCore/Framework/interface/Event.h"
@@ -102,7 +103,7 @@ private:
MonitorElement* h_deltaEndStrip_{nullptr};
// Event Data
- edm::EDGetTokenT<edmNew::DetSetVector<SiStripApproximateCluster>> approxClustersToken_;
+ edm::EDGetTokenT<SiStripApproximateClusterCollection> approxClustersToken_;
edm::EDGetTokenT<edmNew::DetSetVector<SiStripCluster>> stripClustersToken_;
const edmNew::DetSetVector<SiStripCluster>* stripClusterCollection_;
@@ -117,7 +118,7 @@ SiStripMonitorApproximateCluster::SiStripMonitorApproximateCluster(const edm::Pa
: folder_(iConfig.getParameter<std::string>("folder")),
compareClusters_(iConfig.getParameter<bool>("compareClusters")),
// Poducer name of input StripClusterCollection
- approxClustersToken_(consumes<edmNew::DetSetVector<SiStripApproximateCluster>>(
+ approxClustersToken_(consumes<SiStripApproximateClusterCollection>(
iConfig.getParameter<edm::InputTag>("ApproxClustersProducer"))) {
tkGeomToken_ = esConsumes();
if (compareClusters_) {
@@ -139,7 +140,7 @@ void SiStripMonitorApproximateCluster::analyze(const edm::Event& iEvent, const e
const auto tkDets = tkGeom->dets();
// get collection of DetSetVector of clusters from Event
- edm::Handle<edmNew::DetSetVector<SiStripApproximateCluster>> approx_cluster_detsetvector;
+ edm::Handle<SiStripApproximateClusterCollection> approx_cluster_detsetvector;
iEvent.getByToken(approxClustersToken_, approx_cluster_detsetvector);
if (!approx_cluster_detsetvector.isValid()) {
edm::LogError("SiStripMonitorApproximateCluster")
@@ -164,11 +165,11 @@ void SiStripMonitorApproximateCluster::analyze(const edm::Event& iEvent, const e
}
int nApproxClusters{0};
- const edmNew::DetSetVector<SiStripApproximateCluster>* clusterCollection = approx_cluster_detsetvector.product();
+ const SiStripApproximateClusterCollection* clusterCollection = approx_cluster_detsetvector.product();
for (const auto& detClusters : *clusterCollection) {
edmNew::DetSet<SiStripCluster> strip_clusters_detset;
- const auto& detid = detClusters.detId(); // get the detid of the current detset
+ const auto& detid = detClusters.id(); // get the detid of the current detset
// starts here comaparison with regular clusters
if (compareClusters_) {
@@ -233,6 +234,7 @@ void SiStripMonitorApproximateCluster::analyze(const edm::Event& iEvent, const e
} // loop on clusters in a detset
} // loop on the detset vector
+
h_nclusters_->Fill(nApproxClusters);
}on the other hand now it still segfaults for me (in |
|
@mmusich Thank you for your comments. |
this is extremely fishy...
I don't see sign of crashes. I think your job was killed. |
Indeed. Sorry for the confusion. Segmentation error is still exist. |
in my private tests it segfaults here: I haven't traced it further back than this. |
|
Can I ask what is the plan for this development regarding the HI data taking? |
|
@jeongeun @makortel so I had a second look and I think that this is segfaulting because in one particular event we're exceeding the maximum depth of the after applying the changes in #42022 (comment) and the patch below diff --git a/RecoLocalTracker/SiStripClusterizer/plugins/SiStripApprox2Clusters.cc b/RecoLocalTracker/SiStripClusterizer/plugins/SiStripApprox2Clusters.cc
index 803c8949f90..a76b11c9f0e 100644
--- a/RecoLocalTracker/SiStripClusterizer/plugins/SiStripApprox2Clusters.cc
+++ b/RecoLocalTracker/SiStripClusterizer/plugins/SiStripApprox2Clusters.cc
@@ -53,9 +53,14 @@ void SiStripApprox2Clusters::produce(edm::StreamID id, edm::Event& event, const
const StripTopology& p = dynamic_cast<const StripGeomDetUnit*>(*det)->specificTopology();
nStrips = p.nstrips() - 1;
+ std::cout << "before pushing back detId:" << detId << " n. clusters: " << detClusters.end() - detClusters.begin() << std::endl;
+ //int counter{0};
for (const auto& cluster : detClusters) {
+ //std::cout << "pushed " << counter << " clusters" << std::endl;
ff.push_back(SiStripCluster(cluster, nStrips));
+ //counter++;
}
+ std::cout << "after pushing back" << std::endl;
}I've re-run wf. 140.58 and I see in the log file of step3 right before the crash: |
|
The sizes of is somehow flawed (or the indexing in SiStripApproximateClusterCollection::beginIndices_ got somehow screwed up).
|
indeed, thanks for the suggestion. #42495 is a re-vamp of this PR with the necessary fixes. @jeongeun you might want to close this one. |
Good catch! (cef181a) |
|
-1 |
|
7 participants
PR description:
Based on the issue "Dataformat compatibility issue for HI SiStrip cluster in RAW" (#39106)
Aim :
Changing Data-format (edmNew::DetSetVectorr) in RAW to be simple-enough for infinite backwards compatibility. -> It has to be readable by all future CMSSW releases.
Re-defining the corresponding final data-types directly in the ApproximatedClusters.
Need to be straightforward to convert from edmNew::DetSetVector
The simplified data format has updated (recommended by Matti in 2022 Sep)
(master...makortel:cmssw:siStripApproximateClusterCollection_v2)
Target : 13_2_X release (current working release : 13_2_0_pre2)
PR validation:
Tested in CMSSW_13_2_0_pre2, the basic test passed in the CMSSW PR instructions