preparing HBHERecHit class for introducing out-of-time pileup correction... #3102
Conversation
|
A new Pull Request was created by @igv4321 for CMSSW_7_1_X. preparing HBHERecHit class for introducing out-of-time pileup correction... It involves the following packages: DataFormats/HcalRecHit @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks. |
| @@ -0,0 +1,104 @@ | |||
| #ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_H | |||
There was a problem hiding this comment.
What's the point of this?
DataFormats can't depend on Alignment packages.
If I parsed this correctly, this is needed for some templated code that would either call or not this set method.
IMHO, it's cleaner just to add a parameter to that template with an argument explicitly controlling this method is called.
A dummy inline setRawEnergy in other Hcal classes is even more transparent.
If you still like it so much, I think this code does not belong in DataFromats.
@Dr15Jones @ktf would ROOT parse this well?
There was a problem hiding this comment.
Hi Slava,
The point is to have the ability to use this function in
a code templated upon the type of rechit: HBHERecHit,
HFRecHit, HORecHit, etc. This is useful because only the
HBHERecHit class has the methods setRawEnergy and eraw.
Inclusion of "Alignment/Geners/interface/IOIsClassType.hh"
does not cause a linker dependency. I don't think there could
be a problem there.
You don't want to have an extra, let say, boolean parameter
in the template (with the idea to use it in a switch) because
the code will not compile.
You don't want to have an extra template parameter in the
template because you would have to specialize the whole template,
and there could be many such templates.
You do not want to have a dummy setRawEnergy / eraw() in other
Hcal classes because a) the users of those classes might not realize
that they are dummy and b) it is not your business to decide what
value dummy eraw() should return -- it should be up to the user. At
the same time, you do not want to hamper eraw() with an additional
argument for those cases when that additional argument is not needed.
That code belongs in DataFormats/HcalRecHit because it is to be used
with various Hcal rechit classes defined in that package. There is no
better place for it.
This class is not parsed by ROOT.
Hope this answers all of the design concerns.
Regards,
Igor
On 04/01/2014 05:46 AM, slava77 wrote:
In DataFormats/HcalRecHit/interface/rawEnergy.h:
@@ -0,0 +1,104 @@
+#ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_HWhat's the point of this?
DataFormats can't depend on Alignment packages.
If I parsed this correctly, this is needed for some templated code that would either call or not this set method.
IMHO, it's cleaner just to add a parameter to that template with an argument explicitly controlling this method is called.
A dummy inline setRawEnergy in other Hcal classes is even more transparent.If you still like it so much, I think this code does not belong in DataFromats.
@Dr15Jones https://github.com/Dr15Jones @ktf https://github.com/ktf would ROOT parse this well?
—
Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3102/files#r11155206.
There was a problem hiding this comment.
Hi Igor,
thanks for the description.
I think this code belongs in the place where the templates are defined, not here in DataFormats.
I will need comments from the core team if this could have implications on maintenance.
There was a problem hiding this comment.
template<T>
void rawEnergySet(T& h, float v){};
rawEnergySet<HBHERecHit>(HBHERecHit& h, float v){h.setRawEnergy(v);}
takes a few less lines and achieves about the same .. maybe I missed somth more subtle
There was a problem hiding this comment.
Yes, it can be done this way. The difference is that,
if in the future one decides to extend other rec hit
classes with non-dummy setRawEnergy / eraw()
methods, rawEnergy.h would have to be updated
as well. As the necessity of this update is not documented
anywhere, I'd prefer to have the code that would
do the right thing automatically.
Regards,
Igor
On 04/01/2014 09:12 AM, slava77 wrote:
In DataFormats/HcalRecHit/interface/rawEnergy.h:
@@ -0,0 +1,104 @@
+#ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_H
|template
void rawEnergySet(T& h, float v){};rawEnergySet(HBHERecHit& h, float v){h.setRawEnergy(v);}
|takes a few less lines and achieves about the same .. maybe I missed somth more subtle
—
Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3102/files#r11161300.
There was a problem hiding this comment.
I think Cint would have a hard time parsing this.
There was a problem hiding this comment.
Slava, please describe the problems you expect. I can not even foresee
a scenario in which that code would actually need anymaintenance.
That piece of code is a pattern -- described in a 13-year old book by
Alexandrescu called "Modern C++ Design: Generic Programming and
Design Patters Applied". AFAIK, nobody discovered any problem with
that pattern.
Regards,
Igor
On 04/01/2014 11:02 AM, slava77 wrote:
In DataFormats/HcalRecHit/interface/rawEnergy.h:
@@ -0,0 +1,104 @@
+#ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_HI see more problems maintaining this piece of code than a simple explicit instantiation replacing a dummy.
—
Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3102/files#r11167030.
There was a problem hiding this comment.
The contents of that header are not intended for parsing by Cint.
This is why the code is in a separate header.
On 04/01/2014 01:28 PM, Chris Jones wrote:
In DataFormats/HcalRecHit/interface/rawEnergy.h:
@@ -0,0 +1,104 @@
+#ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_HI think Cint would have a hard time parsing this.
—
Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3102/files#r11173865.
There was a problem hiding this comment.
Dear Igor,
I can just repeat my previous comments here.
Please remove this from the data formats.
Please consider using simple explicit partial template function instantiation.
Thank you.
There was a problem hiding this comment.
OK, rawEnergy.h will be moved somewhere else.
New pull request is #3150.
On 04/01/2014 07:51 PM, Slava Krutelyov wrote:
In DataFormats/HcalRecHit/interface/rawEnergy.h:
@@ -0,0 +1,104 @@
+#ifndef DATAFORMATS_HCALRECHIT_RAWENERGY_HDear Igor,
I can just repeat my previous comments here.
Please remove this from the data formats.
Please consider using simple explicit partial template function instantiation.Thank you.
—
Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/3102/files#r11189181.
Added a member to the HBHERecHit to store uncorrected energy
Added a setter/getter function for the uncorrected energy that can
be used with classes that do not have relevant members
Modified HcalRecHitDump for grepping convenience