Properly compute PFRecHit position for HGCAL #20631
Conversation
The current CaloGeometry of HGCAL is not capable of handling the full granularity of the detector. Hence, for all the detids belonging to the same wafer, the mid position of the wafer is returned, instead of the more accurate pad's position. This PR introduces a small adapter for the HGCAL CaloCell that internally stores the correct position for each pad and returns it via the usual interface. No changes are required anywhere in the code except for the code responsible to import HGCAL RecHit as PFRecHits. This is a hack that will disappear as soon as a proper CaloGeometry for the HGCAL detector is in place.
|
The code-checks are being triggered in jenkins. |
|
The reason to develop this PR surfaced at one of the last UPSG meeting, where an extremely poor agreement between the SC and the linked gsf track has been shown for electrons in HGCAL slide 7 at this link.
This PR, when run on ZEE sample, 0PU, will restore a more proper situation:
|
|
+code-checks |
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages: Geometry/CaloGeometry @perrotta, @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @slava77 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. |
|
+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:
|
|
assign upgrade |
|
New categories assigned: upgrade @kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
@rovere : I miss the point why you need to cache the whole caloCells_ vector if you only need one thisCell at once... |
|
@perrotta I build one PFRecHit at a time, but each time the PFRecHit stores internally the pointer to thisCell, so I need to keep track of each of them. |
|
@rovere - you could delegate this tracking to a smart pointer :-) |
|
@ianna no, I cannot. |
|
+1 ok, we'll come back to this question when geometry has been migrated to smart pointers. |
|
+1 |
|
+1
|
cmsbuild
added
fully-signed
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 (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
I ran also with 4 threads and I see a 1.5GB increment in the memory peak. @kpedro88 |
|
@slava77 I'm not sure about production memory usage for 930. We've been looking at relvals so far, which have much higher memory usage. |
|
|
||
| class CaloCellGeometryHGCALAdapter : public FlatTrd { | ||
| public: | ||
| explicit CaloCellGeometryHGCALAdapter (const FlatTrd * f, GlobalPoint p) : FlatTrd(*f), position_(p) {} |
There was a problem hiding this comment.
why is a copy of the FlatTrd object needed here (rather than keeping the pointer) - sorry to add noise.
There was a problem hiding this comment.
@davidlange6 the PFRecHit has a member CaloCellGeometry const * caloCell_. We have to provide it with something equivalent - this new class inherits from FlatTrd which inherits from CaloCellGeometry, but returns the correct position. I thought about it for a while and couldn't come up with a better hack (i.e. without changing the PFRecHit class).
There was a problem hiding this comment.
Ok so why does this not work? [I think the savings is not so substantial as CaloCellGeometry is the majority of the size of FlatTrd so lets save the question for later
class CaloCellGeometryHGCalAdapter : public CaloCellGeometry {
public:
...
private:
const FlatTrd *trd_;
GlobalPoint position_;
|
and storing a pointer rather than a copy does not work? FlatTrd is a pretty complex class to make copies of. ok - just asking.
… On Sep 27, 2017, at 12:25 PM, Kevin Pedro ***@***.***> wrote:
@kpedro88 commented on this pull request.
In Geometry/CaloGeometry/interface/CaloCellGeometryHGCALAdapter.h:
> @@ -0,0 +1,16 @@
+#ifndef GeometryCaloCellGeometryHGCALAdapter
+#define GeometryCaloCellGeometryHGCALAdapter
+
+#include "Geometry/CaloGeometry/interface/FlatTrd.h"
+
+class CaloCellGeometryHGCALAdapter : public FlatTrd {
+public:
+ explicit CaloCellGeometryHGCALAdapter (const FlatTrd * f, GlobalPoint p) : FlatTrd(*f), position_(p) {}
@davidlange6 the PFRecHit has a member CaloCellGeometry const * caloCell_. We have to provide it with something equivalent - this new class inherits from FlatTrd which inherits from CaloCellGeometry, but returns the correct position. I thought about it for a while and couldn't come up with a better hack (i.e. without changing the PFRecHit class).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@davidlange6 what would such a class look like? It still has to inherit from and behave like FlatTrd - all the memory for its members will be allocated even if we keep a pointer to the original instead of copying. |
|
The obj I return must be a valid CaloCell since I have no guarantee that
nobody will call other methods rather than position. It has to have
CaloCell interface, actually be a CaloCell.
Ciao,
-- Marco.
…___________
Marco Rovere
Marco.Rovere@cern.ch
CERN EP-CMG | room 40 3-A28 | tel +41227671209 (71209)
On Wed, Sep 27, 2017 at 5:35 PM, Kevin Pedro ***@***.***> wrote:
@davidlange6 <https://github.com/davidlange6> what would such a class
look like? It still has to inherit from and behave like FlatTrd - all the
memory for its members will be allocated even if we keep a pointer to the
original instead of copying.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20631 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABHaR5zkLHw7W3_p4UVWdfGxBEZRsgFmks5smmtEgaJpZM4Pgz7w>
.
|
|
ok, if you have to inherit from FlatTrd, then my question is moot. (but duplicating this information should be addressed longer term..)
… On Sep 27, 2017, at 12:35 PM, Kevin Pedro ***@***.***> wrote:
@davidlange6 what would such a class look like? It still has to inherit from and behave like FlatTrd - all the memory for its members will be allocated even if we keep a pointer to the original instead of copying.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@bsunanda is working on a real fix. It will be much more involved, but should avoid the memory implications here. |
|
On 9/27/17 8:39 AM, Marco Rovere wrote:
The obj I return must be a valid CaloCell since I have no guarantee that
nobody will call other methods rather than position. It has to have
CaloCell interface, actually be a CaloCell.
what does it mean to all other methods?
will they return the center of the wafer [if there is an alternative way
to get positions or corners]?
Just trying to understand how complete/safe is this fix.
|
|
PFRecHit stores a pointer to a CaloCell and it can call whatever methods
from it. I need to be sure to pass a valid CaloCell, with the correct
position. That's what I meant.
Ciao,
-- Marco.
…___________
Marco Rovere
Marco.Rovere@cern.ch
CERN EP-CMG | room 40 3-A28 | tel +41227671209 (71209)
On Wed, Sep 27, 2017 at 6:19 PM, Slava Krutelyov <notifications@github.com>
wrote:
On 9/27/17 8:39 AM, Marco Rovere wrote:
> The obj I return must be a valid CaloCell since I have no guarantee that
> nobody will call other methods rather than position. It has to have
> CaloCell interface, actually be a CaloCell.
>
what does it mean to all other methods?
will they return the center of the wafer [if there is an alternative way
to get positions or corners]?
Just trying to understand how complete/safe is this fix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20631 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABHaR7LKUySE-yltm4QrsiRk4wx8ilGkks5smnV5gaJpZM4Pgz7w>
.
|
|
merge |
The current CaloGeometry of HGCAL is not capable of handling
the full granularity of the detector. Hence, for all the
detids belonging to the same wafer, the mid position of the
wafer is returned, instead of the more accurate pad's
position. This PR introduces a small adapter for the HGCAL
CaloCell that internally stores the correct position for
each pad and returns it via the usual interface. No changes
are required anywhere in the code except for the code
responsible to import HGCAL RecHit as PFRecHits. This is a
hack that will disappear as soon as a proper CaloGeometry
for the HGCAL detector is in place.
Forward port of #20630