TICL Reorganization, reloaded. #27160
Conversation
|
The code-checks are being triggered in jenkins. |
|
@slava77 for some weird reason PR #27026 got a little confused after my last rebase. I decided to close that and open a fresh new PR, hoping that this time the review process will go smoother. As for moving stuff out of DataFormats/TICL into DataFormats/HGCRecHits, I disagree. That package has a very specific name and I'd not mix stuff together. I see the value of not adding too many packages, yet the proposed solution is not optimal, from my point of view. Either we change the name of the current DataFormats/HGCRecHit into something more generic, like HGCRecoCommonn, or any other name you could suggest, or I'd prefer to keep things logically separated into different folders. I'll go through the other comments in the coming days. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27160/10308
|
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages: DataFormats/TICL The following packages do not have a category, yet: DataFormats/TICL @perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@slava77 Also for the move of TICL algorithms into RecoLocalCalo, I believe this is misleading. TICL, or more in general HGCAL reconstruction, will have internally links and connections with other detectors, like tracker/muons. I'd prefer to have it in a separate package that explicitly declares its purpose. |
|
please test |
I was apparently incorrect that the current status of this package represents the target as a skeleton. About the DataFormats. Please remind me the motivation to move the transient-only products to the DataFormats subsystem. |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
Comparison job queued. |
cmsbuild
added
comparison-pending
tests-approved
and removed
comparison-available
tests-started
labels
|
+1
@kpedro88 please check this PR at your earliest so that we can then proceed with #27194 |
| ticl::constants::nPhiBins, | ||
| layerClusters, | ||
| mask, | ||
| 2, |
There was a problem hiding this comment.
could these magic numbers be named constants?
| std::copy(std::begin(effective_cluster_idx), std::end(effective_cluster_idx), | ||
| std::back_inserter(tmp.vertices)); | ||
| tmp.vertex_multiplicity.resize(effective_cluster_idx.size(), 0); | ||
| std::copy(std::begin(effective_cluster_idx), std::end(effective_cluster_idx), std::back_inserter(tmp.vertices)); |
There was a problem hiding this comment.
I think this would be simpler (and maybe better performing) as:
tmp.vertices.insert(tmp.vertices.end(),effective_cluster_idx.begin(),effective_cluster_idx.end());
| sort(idx.begin(), idx.end(), | ||
| [&v](size_t i1, size_t i2) {return v.clusters()[i1]->z() < v.clusters()[i2]->z();}); | ||
| std::iota(std::begin(idx), std::end(idx), 0); | ||
| sort( |
|
+upgrade |
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
Comparison is ready Comparison Summary:
|
|
@fabiocos |
|
On 6/26/19 4:56 PM, Fabio Cossutti wrote:
I will integrate this PR in next IB.
Thank you.
|
|
+1 |
PR description:
TICL Reorganization
Several changes entered this PR.
DataFormats/TICL, that has to be added as a CMSSW package.
Tracksters and Tile are currently present. The encapsulation of
std::array as a transient data member of a Tile is due to the fact
that ROOT natively supports those and refuses to create ad-hoc
dictionaries. Moreover, at present, an std::array cannot sit at the
top of a ROOT branch requiring, once again, its encapsulation.
containing all the 2D reconstructed clusters is created once and put
into the Event. All iterations will fetch it from the event and mask
the 2D clusters on the fly. This required also some changes to the
clusters filters logic. Now the filters produce also a vector
that represents the result of merging their input mask with their
own selection. TICL iterations will make use of the filter's
vector to dynamically mask clusters from the "master Tile".
energy of 2D clusters is flat and based on their multiplicity, i.e,
a fraction of energy/multiplicity is assigned to all Tracksters that
use the 2D clusters. Better splitting algorithms can be studies or
Machine Learning techniques could be put at work here.
This required an additional loop over the components of each 2D
cluster. In the future the CaloCluster, PFCluster and hits and
fractions data structures could be reviewed or, at least, TICL could
use some other, more efficient, internal data representation and
collapse into the legacy ones only at the very last stage.
PR validation:
runTheMatrix.py -l limited -i all -j 4and other samples.
Note to the reviewers: all changes in
RecoLocalCalo/HGCalRecAlgosare purely due to the code-format. No other changes have been committed there. I did so so that PR #26991 could be closed (if and when this PR gets merged)