Produce multiple SimCluster collections in CaloTruthAccumulator#50578
Produce multiple SimCluster collections in CaloTruthAccumulator#50578tcuisset wants to merge 3 commits intocms-sw:masterfrom
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50578/48754
|
|
A new Pull Request was created by @tcuisset for master. It involves the following packages:
@civanch, @cmsbuild, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| void clearHitsAndFractions() { | ||
| hits_.clear(); | ||
| fractions_.clear(); | ||
| } |
There was a problem hiding this comment.
I did not know of this idiom, thanks ! Though it seems weird to me to de-allocate the entire vector before processing the next event, it would need to reallocate every time. Maybe calling hits_.shrink_to_fit(); after filling would be more efficient, but I don't know if there is a real memory issue here (these are only vectors of int/float)
There was a problem hiding this comment.
This pattern is recommended when there is the potential for memory hoarding, which it seems like there could be in this case. There is indeed a tradeoff between reallocating and hoarding: reallocating makes the job slower, but hoarding can cause syskill if the job exhausts available memory.
There was a problem hiding this comment.
Concerning shrink_to_fit(), note that it is non-binding, so it might as well do nothing.
Regarding the reallocation point, I'm also curious to know what is the most efficient way to do this in CMSSW.
There was a problem hiding this comment.
It seems the clearHitsAndFractions function is only ever called once, by CaloTruthAccumulator, and for that use we are essentially clearing and refilling with the same length, so shrinking is not good idea.
There was a problem hiding this comment.
Are CaloParticles really held from one event to the next?
There was a problem hiding this comment.
Sorry, I got confused between the vector<CaloParticle> and the vector<int> hits_ that is inside the CaloParticle. The former is kept per stream (just cleared), but the CaloParticle are not kept
There was a problem hiding this comment.
Regarding the reallocation point, I'm also curious to know what is the most efficient way to do this in CMSSW.
Within a module processing one Event, avoiding reallocations is usually a good idea (especially within loops).
Between Events, it is better to release the event, because memory hoarders are a big problem (as Kevin already wrote). One allocation per module per event is less than a rounding error when we have components doing tens of millions of memory allocations per Event.
| template <typename R> | ||
| requires std::ranges::input_range<R> && std::same_as<std::ranges::range_value_t<R>, SimCluster> | ||
| SimCluster SimCluster::mergeHitsFromCollection(R const &inputs) { |
There was a problem hiding this comment.
Just a curiosity: why do you need the template at all, given that you require the SimCluster type for the elements of the iterable? Is this going to be used for different containers in your next PR? Aren't they all going to be of the SimClusterCollection type?
There was a problem hiding this comment.
Indeed probably the template is not needed, but I had in mind that we may want to merge CaloParticle dataformat as well. With hindsight it was probably not needed, but since I already coded it and it works....
|
|
||
| ret.hits_.reserve(acc_fractions.size()); | ||
| ret.fractions_.reserve(acc_fractions.size()); | ||
| for (auto hit_and_fraction : acc_fractions) { |
| class SubClusterMergerBase { | ||
| public: | ||
| /// Called when a SimCluster is started (along with the index of the SimCluster in its output collection) | ||
| void start_subcluster(DecayChain::edge_descriptor e, const DecayChain &g, std::size_t currentSubClusterIndex); |
There was a problem hiding this comment.
what is the type/content of edge_descriptor? is it a heavy object that should be passed by reference?
There was a problem hiding this comment.
It is a pair of unsigned long (indices of source and child vertex of the edge), so it can be passed by value
| ClusterParentIndexRecorderT caloParticleParentIndexRecorder, | ||
| ClusterParentIndexRecorderT subClusterToMergedClusterParentIndexRecorder, |
There was a problem hiding this comment.
should these be passed by reference?
There was a problem hiding this comment.
These are always built with temporaries, so they should automatically get moved.
From here a temporary ClusterParentIndexRecorder is made :
std::make_tuple(simClusterMergerFastJet_config_.getMerger(
ClusterParentIndexRecorder(simClusterMergerFastJet_config_.clustersToCaloParticleMap,
caloParticles_refProd_),
ClusterParentIndexRecorder(simClusterMergerFastJet_config_.subClustersToMergedClusterMap,
boundarySimCluster_refProd_))))));| legacySimClusters_config_(producesCollector, "MergedCaloTruth"), | ||
| boundarySimClusters_config_(producesCollector, "MergedCaloTruthBoundaryTrackSimCluster"), | ||
| caloParticleSimClusters_config_(producesCollector, "MergedCaloTruthCaloParticle"), |
There was a problem hiding this comment.
for my understanding, how much duplication in content is there between these three products?
There was a problem hiding this comment.
there is some duplication, since mostly the same hits are stored.
"legacy" and "boundary" simClusters are nearly identical (except some corner cases involving photons). Ultimately I think we should get rid completely of "legacy" SimCluster, but there is still code depending on it for now (though I plan for TICL to remove all dependency on "legacy").
The "CaloParticle-SimCluster" is indeed a duplicate of "CaloParticle". This is again a transition, I think it would be more convenient to use one dataformat for everything since SimCluster & CaloParticle are so similar. Ultimately we could migrate everything and get rid of "CaloParticle" as a dataformat, but it is a large change for a single PR
There was a problem hiding this comment.
Indeed, for "breaking" changes, we have the opportunity to integrate them in the upcoming CMSSW_20 cycle (or later) without needing to retain backward compatibility for Run ≤3. So as preparatory work for that kind of migration, this is acceptable.
Still, I wonder: is it possible to have switches or workflows that disable some of these collections, if they are not all needed for all use cases?
There was a problem hiding this comment.
I could implement a switch if needed. Which options would be needed ? I was mostly thinking that we may want to turn off CaloTruthAccumulator completely when we don't care about calorimeter validation, rather than really wanting to tweak which collections get produced
There was a problem hiding this comment.
From the description, it seems like legacy/new/both would be a valid set of choices.
There was a problem hiding this comment.
I've implemented two switches: produceLegacySimCluster and produceBoundaryAndMergedSimCluster
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50578/48756
|
|
Pull request #50578 was updated. @civanch, @cmsbuild, @kpedro88, @mdhildreth can you please check and sign again. |
CaloTruthAccumulator is updated to produce multiple SimCluster collections: - CaloParticle collection - SimCluster "CaloParticle" collection (same thing but in SimCluster dataformat, simplifies consumers) - SimCluster "legacy" collection (identical behaviour as SimCluster before this commit) - SimCluster "boundary" collection : creates a SimCluster for every SimTrack crossing calorimeter boundary (and having cumulative simhits from itself or its simtrack descendents). Can be used in replacement of "legacy" collection (the only difference is for primary photons that do not convert before boundary). - SimCluster "merged" collection : merges very collimated SimTrack into one SimCluster (only from same CaloParticle). For now it uses fastjet anti-kt with very small distance parameter (0.05) It also produces RefVector to map SimCluster to CaloParticle
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50578/48757
|
|
Pull request #50578 was updated. @civanch, @cmsbuild, @kpedro88, @mdhildreth can you please check and sign again. |
| void clear() { | ||
| outputClusters.clear(); | ||
| clustersToCaloParticleMap.clear(); | ||
| } |
There was a problem hiding this comment.
IIUC this clear() gets called in initializeEvent(), which hints that the contents would be kept alive from one Event to the next. If this is the case, it would be much better to release the memory at the end of the Event (i.e. in finalizeEvent(), and really release, not just clear()).
There was a problem hiding this comment.
Ah, on further look I see they are moved in
cmssw/SimGeneral/CaloAnalysis/plugins/CaloTruthAccumulator.cc
Lines 741 to 744 in 1fb6bb5
So the memory hoarding doesn't occur, never mind this comment.
There was a problem hiding this comment.
In finalizeEvents the vectors outputClusters and clustersToCaloParticleMap are moved into the event with event.emplace, which I believe would leave them without any memory of their own (though it does not seem to be a C++ standards requirements). The clear is actually more to get the vector in a determined state (and not in "moved-from" undetermined state), it should already be at zero capacity at this point.
But probably adding a shrink_to_fit would not hurt
There was a problem hiding this comment.
(ah wrote my comment at the same time as yours)
| applyToSimClusterConfig([&event](auto &config) { | ||
| event.emplace(config.outputClusters_token, std::move(config.outputClusters)); | ||
| event.emplace(config.clustersToCaloParticleMap_token, std::move(config.clustersToCaloParticleMap)); | ||
| }); | ||
| event.emplace(simClusterMergerFastJet_config_.subClustersToMergedClusterMap_token, | ||
| std::move(simClusterMergerFastJet_config_.subClustersToMergedClusterMap)); |
There was a problem hiding this comment.
I think it would be clearer for a future reader if the default-constructed state of all of these moved-from objects would be restored here after the emplace() calls instead of in initializeEvent() (to avoid raising alarms for possible memory hoarding).
I guess the swap() pattern would give best guarantees the memory to be actually released.
| std::unordered_map<Index_t, float>().swap(m_detIdToTotalSimEnergy); | ||
| std::unordered_multimap<Barcode_t, Index_t>().swap(m_simHitBarcodeToIndex); | ||
| m_detIdToTotalSimEnergy.clear(); |
There was a problem hiding this comment.
The clear() does not release memory, I'd suggest to keep the existing swap() pattern.
…d at the end of each event
…new" (boundary, merged, CaloParticle-SimCluster) SimCluster collections
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50578/48761
|
|
Pull request #50578 was updated. @civanch, @cmsbuild, @kpedro88, @mdhildreth can you please check and sign again. |
|
please test |
|
+1 Size: This PR adds an extra 16KB to repository
Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
| config.clearAndReleaseMemory(); | ||
| }); | ||
| if (produceBoundaryAndMergedSimCluster_) | ||
| event.emplace(simClusterMergerFastJet_config_.subClustersToMergedClusterMap_token, |
There was a problem hiding this comment.
have you tested all the cases of enabling and disabling these produce switches? the produces<>() command is always called, but the collection is only added to the event conditionally if produceBoundaryAndMergedSimCluster_ is true
There was a problem hiding this comment.
I have tested it, the framework seems fine with calling "produces" but not actually putting anything in the event (I think this is how HLT works also)
There was a problem hiding this comment.
If it is known at a module construction time that some data product won't be produced at all in a job, it would be better to avoid the produces() call as well.
|
are the comparison changes in |
6 participants
PR description:
CaloTruthAccumulator is updated to produce multiple SimCluster collections, to simplify the validation code and allow easier interpretation of simulation truth. The collections produced are:
It also produces RefVector to map SimCluster to CaloParticle.
This PR only contains the change in CaloTruthAccumulator. Another PR will follow with the TICL validation changes to use the new SimClusters.
Documentation
Slides at TICL meeting
Slide 7 of HGCAL presentation at NGT MC truth workshop
PR validation:
Ran usual PR tests, and tested with an updated TICL validation (to be submitted to a future PR)
No changes in physics are expected from this PR, since the new collections are not used (for now).