Some EcalClusterTools maintainance and energy matrix in Egamma MVA Ntuplizers #25633
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25633/7950
|
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages: RecoCaloTools/Navigation @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| } | ||
| // slow elegant version |
There was a problem hiding this comment.
I was a bit surprised by this comment here: why was that solution so slow? Is looping over the DetIDs twice and keeping a vector of them for the second loop so expensive? That statement prompted me to do this rectangle range solution where you only loop over the detIDs once. I hope it combines the speed of the previous solution with the "elegance" of the commented-out solution.
There was a problem hiding this comment.
So this solution is slow because a cluster can own many many hits due to how PF clustering works. As you know, PF clustering shares the energy of a crystal between local energy maximums. It does this fairly globally and (initially at least) did not clean well. So you would end up with half the ECAL assigned as a "rec-hit" of the cluster but with fraction of 1E-8 or similar. This was later cleaned up to be >1E-4/E-5 (and may have been further changed) but at the time of writing you were running over a huge number of hits, hence the "slow" comment. Its not clear to me if this was actually measured to be slow or just thought to be from the above argument, I think it was actually slow from dimly remembered conversations.
One thing which I want to achieve in the shutdown is to review the minimum fraction (or fraction*energy) for a hit to be part of a PFCluster to further reduce this problem. Although due to the raised minimum fraction this may not be a problem anymore.
There was a problem hiding this comment.
That is interesting, thanks for explaining! But I was talking about a different thing. You explained why recHitEnergy() can be slow, right? I was more talking about the fact that the comments labels this as slow (if you "unwrap" the functions and remove the frac related stuff):
CaloNavigator<DetId> cursor = CaloNavigator<DetId>( id, topology->getSubdetectorTopology( id ) );
std::vector<DetId> v;
for ( int i = ixMin; i <= ixMax; ++i ) {
for ( int j = iyMin; j <= iyMax; ++j ) {
cursor.home();
cursor.offsetBy( i, j );
if ( *cursor != DetId(0) ) v.push_back( *cursor );
}
}
float energy = 0;
for ( std::vector<DetId>::const_iterator it = v.begin(); it != v.end(); ++it ) {
energy += recHitEnergy( *it, recHits );
}
and this as _fast:
float energy = 0;
CaloNavigator<DetId> cursor = CaloNavigator<DetId>( id, topology->getSubdetectorTopology( id ) );
std::vector<DetId> v;
for ( int i = ixMin; i <= ixMax; ++i ) {
for ( int j = iyMin; j <= iyMax; ++j ) {
cursor.home();
cursor.offsetBy( i, j );
energy += recHitEnergy( *cursor, recHits )
}
}
In the "slow" solution, the DetIds in the rectangle are precomputed and stored, and in the "fast" solution the energies are already added in the loop where the DetHits are obtained (and the DetHits are not stored). If one believes that this really makes a difference, it made sense to implement this CaloRectangleRange I thought.
There was a problem hiding this comment.
Ah you're right, I miss read the code. Okay, I have no idea :)
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
Sorry I have no idea where these differences come from! I didn't see them neither in comparisons of the egamma MVA nutuplizer outputs (comparing to a more minimalist implementation of getting the energy matrix), nor in the outputs of the |
cmsbuild
added
code-checks-pending
comparison-pending
and removed
code-checks-approved
comparison-available
tests-approved
labels
| class ElectronMVANtuplizer : public edm::one::EDAnalyzer<edm::one::SharedResources> { | ||
| public: | ||
| explicit ElectronMVANtuplizer(const edm::ParameterSet&); | ||
| ~ElectronMVANtuplizer() override; |
There was a problem hiding this comment.
Even if it is a "one" module, why to remove the destructor?
There was a problem hiding this comment.
A counter argument, why declare it if the compiler can write the default?
There was a problem hiding this comment.
I would naively say that otherwise the not overridden dtor of the base class is used instead, and this would fail in deleting specific members of the derived one: isn't it so? Or a virtual method is always overridden by a default one in the derived class?
There was a problem hiding this comment.
The compiler will aways write a destructor for a class if you do not specifically state you will write the destructor. This is also true for virtual inheritance. I.e. the compiler will write the correct destructor for ElectronMVANtuplizer which will call the destructors for all member data of ElectronMVANtuplizer.
There was a problem hiding this comment.
Thank you @Dr15Jones ! Comment retracted, then
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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) |
|
please test the random unit test errors should be gone |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-rejected
labels
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 the updated test configurations run smoothly |
Hi,
I always felt the potential of machine learning applied to the ECAL is not fully tapped, while it's 2 dimensional nature provides a very good playing ground to test modern neural network architectures designed for image processing.
To enable quick studies in this direction for myself and everybody, I always wanted to (optionally) add the raw rec hit energies in a N times N matrix around the seed crystal to the Electron/PhotonMVANtuplizers. This is now possible, with N being a configurable parameter.
For this purpose, I wrote a new function
EcalClusterLazyTools<T>::getEnergiesand generally went over the EcalClusterTools/EcalClusterLazyTools duo, tying to make it a bit more slick. This includes:templatelines (see the lazy tools)