Phase2 - HGC - Realistic SimClusters #19571
Conversation
|
A new Pull Request was created by @felicepantaleo (Felice Pantaleo) for master. It involves the following packages: RecoParticleFlow/PFClusterProducer @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
assign upgrade |
|
New categories assigned: upgrade @kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
There was a problem hiding this comment.
Code needs some work to get into shape to be merged in official-cmssw. A few other general questions/comments:
- Are there performance plots showing what effect this has on clusters?
- We need to understand the CPU and memory performance of this code - even if it's not enabled right now, it will be eventually.
|
|
||
| RealisticCluster() | ||
| { | ||
| totalEnergy = 0.f; |
There was a problem hiding this comment.
use member initializer list in constructor
| { | ||
| hitIdsAndFractions_.emplace_back(hit,fraction); | ||
| } | ||
| //TODO: hits are sorted, replace with binary search. |
There was a problem hiding this comment.
Are the hits already sorted in this PR? If so, find_if could just be replaced with lower_bound
|
|
||
| namespace | ||
| { | ||
| //TODO: get number of layers per subdet from geometry |
There was a problem hiding this comment.
please do this ASAP, the methods are available now (since #19485)
There was a problem hiding this comment.
I cannot find a method which returns the number of layers.
Should I really use something like the following?
`auto geomBH = static_cast<const HGCalGeometry*>(geom_->getSubdetectorGeometry(DetId::Forward,ForwardSubdetector::HGCHEB));
bhOffset_+ (geomBH->topology().dddConstants()).layers(true);`
| void insertHitPosition(float x, float y, float z, unsigned int hitIndex) | ||
| { | ||
| hitPosition_[hitIndex] = | ||
| { |
There was a problem hiding this comment.
remove unnecessary spaces/linebreaks here
| float getDecayLength(unsigned int layer) | ||
| { | ||
| if (layer <= 28) | ||
| return 2.f; |
There was a problem hiding this comment.
decay length magic numbers should be made into consts with explanatory names
| //TODO: get number of layers+1 from geometry | ||
| realisticAssociator.init(hits.size(), simClusters.size(), numberOfLayers + 1); | ||
| // for quick indexing back to hit energy | ||
| std::unordered_map < uint32_t, size_t > detIdToIndex(hits.size()); |
There was a problem hiding this comment.
need #include <unordered_map> for this
| for (unsigned int ic = 0; ic < simClusters.size(); ++ic) | ||
| { | ||
| const auto & sc = simClusters[ic]; | ||
| auto hitsAndFractions = std::move(sc.hits_and_fractions()); |
There was a problem hiding this comment.
the std::move() is probably unnecessary here, the compiler will optimize this
| } | ||
| realisticAssociator.computeAssociation(_exclusiveFraction, _useMCFractionsForExclEnergy); | ||
| realisticAssociator.findAndMergeInvisibleClusters(_invisibleFraction, _exclusiveFraction); | ||
| auto realisticClusters = std::move(realisticAssociator.realisticClusters()); |
There was a problem hiding this comment.
just const auto&, no std::move()
| output.emplace_back(); | ||
| reco::PFCluster& back = output.back(); | ||
| edm::Ref < std::vector<reco::PFRecHit> > seed; | ||
| auto hitsIdsAndFractions = std::move(realisticClusters[ic].hitsIdsAndFractions()); |
There was a problem hiding this comment.
just const auto&, no std::move()
| #include "SimDataFormats/CaloAnalysis/interface/SimClusterFwd.h" | ||
|
|
||
| class RealisticSimClusterMapper : public InitialClusteringStepBase { | ||
| typedef RealisticSimClusterMapper B2DGT; |
There was a problem hiding this comment.
What is the reason for this indecipherable typedef?
|
Dear @kpedro88 ,
We have to tune configuration parameters and use PR #19572 to filter out rechits going into the cluster based on energy and distance. |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
1 similar comment
|
@felicepantaleo and then what did you do before making the latest "removing white space" commits? The merge-topic commit wasn't there after the rebase, it appeared when you pushed the new commits. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
@kpedro88 I created another pre3 area, merged the branch and checked that everything was ok... this has now already been rebased to CMSSW_9_3_0_pre3 |
|
That is not a correct use of |
|
Comparison is ready Comparison Summary:
|
|
@perrotta
https://fpantale.web.cern.ch/fpantale/PRs/19571/diff_timeMem.html |
|
Thank you @felicepantaleo ! Trying to dig into your diffs:
Overall, I think that you should understand where the doubling of the number of electrons comes from, and either justify or try to tune it, so that no extra CPU is eaten neadlessly |
|
@perrotta
|
|
Could you provide some evidence that the newly produced electrons are really worth being produced and saved? I mean, some eff vs fakes, even with the low stat that you have in your sample. Is any tuning, at least in the endcaps, able save the physics content and reduce the additional cpu time required by this PR? |
|
ciao Andrea, Efficiency is 100% by definition, since all the PFclusters are associated to a SimCluster. What could be added (in another PR) is an energy cut, for which if a PFCluster has energy below, say 1GeV, I set it to zero and remove all the hits contained in it.... |
|
+1
|
|
@kpedro88 |
|
+1 |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
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, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
This PR creates PFClusters using a more realistic assignment of energy fractions to Clusters based on distance and energy.
It also allows to merge two simclusters together if they share an amount of energy above threshold.
This PR does not enable Realistic SimClusters by default, so downstream quantities should remain unchanged.
You can find a presentation with more details here:
https://indico.cern.ch/event/647401/contributions/2632485/attachments/1479578/2293729/20170620_Felice_Realism.pdf