HBHE: obsolete code cleanup #21289
HBHE: obsolete code cleanup #21289
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @mariadalfonso for master. It involves the following packages: DataFormats/HcalRecHit @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
My understanding is that there are going to be only "no PU mitigation" (M0 energy now), "HLT energy" (now M3), and "offline energy with PU mitigation" (now M2). |
| @@ -54,6 +57,7 @@ class HBHERecHit : public CaloRecHit { | |||
| float chiSquared_; | |||
| float rawEnergy_; | |||
| float auxEnergy_; | |||
| float addEnergy_; | |||
There was a problem hiding this comment.
what is the logic for this name "add"?
There was a problem hiding this comment.
"add" stay for additional.
The final idea is to have the M0 (no PU mitigation) and Mahi (in replacement of both M2 and M3).
This additional energy is mainly for cross testing M3 vs M2 vs Mahi.
Let's discuss how we want to do this at the next RECO meeting (17th november)
There was a problem hiding this comment.
auxiliary also means additional.
So, instead of expanding by synonyms, maybe aux1Energy (just the first index) or aux2Energy (to imply method2).
Perhaps it's time to give a bit more substance to the name to know what's inside.
If this is needed for comparisons, is it obvious that just energy is enough and you wouldn't need chi2, time, or some other flags?
Still, if this is needed for one pre-release, the addition of extra parameter(s) is rather wasteful. It seems like it is possible to do much more for comparisons by having another collection in parallel.
|
+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:
|
|
Sure, M2 and MAHI will not run together in production. Eventually, if/when MAHI reaches its timing goals, it will also replace M3. Nevertheless, it is useful to have this member in order to support
all kinds of rechit energy comparisons, removing the need to make multiple rechit collections and simplifying both reco configuration and further analysis.
…On 11/13/2017 09:31 AM, Slava Krutelyov wrote:
Add extra energy in the HBHERecHit Data Format
Needed since for 10_0_X we will have three methods to compare M2/M3/Mahi
My understanding is that there are going to be only "no PU mitigation" (M0 energy now), "HLT energy" (now M3), and "offline energy with PU mitigation" (now M2).
There is going to be no support (beyond private tests and relvals) for running two versions of the offline energy measurement with PU mitigation (M2 and MAHI will not run together in production).
If this is correct, I do not see a well justified need to add a new energy data member.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcms-sw%2Fcmssw%2Fpull%2F21289%23issuecomment-343955251&data=02%7C01%7Ci.volobouev%40ttu.edu%7C27eec107634145786d4e08d52aaba2a6%7C178a51bf8b2049ffb65556245d5c173c%7C0%7C0%7C636461839013090452&sdata=ezujfrDglCcsfemDQ6ReFzWxNAL29JSNEkoncfQO%2FUA%3D&reserved=0>,
or mute the thread
<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFS4-aXn9nKVK2TxiQMVn30HcD99TQIuks5s2GDYgaJpZM4Qbv5i&data=02%7C01%7Ci.volobouev%40ttu.edu%7C27eec107634145786d4e08d52aaba2a6%7C178a51bf8b2049ffb65556245d5c173c%7C0%7C0%7C636461839013090452&sdata=TinY0XgW8tXg5QzYuhlN8iLNOP1IhwZ%2BDgfUf3n7Tps%3D&reserved=0>.
|
|
Minor comment: extraEnergy (?)
https://dictionary.cambridge.org/dictionary/english/extra
usually "add" is used in method names as an action verb describing addition
of quantities(?)
…On Mon, 13 Nov 2017, mariadalfonso wrote:
@mariadalfonso commented on this pull request.
________________________________________________________________________________________________________________________________________
In DataFormats/HcalRecHit/interface/HBHERecHit.h:
> @@ -54,6 +57,7 @@ class HBHERecHit : public CaloRecHit {
float chiSquared_;
float rawEnergy_;
float auxEnergy_;
+ float addEnergy_;
"add" stay for additional.
The final idea is to have the M0 (no PU mitigation) and Mahi (in replacement of both M2 and M3).
This additional energy is mainly for cross testing M3 vs M2 vs Mahi.
Let's discuss how we want to do this at the next RECO meeting (17th november)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.[AEx02ia_y77-HjSEBFP9ZZFqW1YDoek9ks5s2LHzgaJpZM4Qbv5i.gif]
|
|
On 11/14/17 7:43 AM, Salavat Abdullin wrote:
Minor comment: extraEnergy (?)
> "add" stay for additional.
both are synonyms of auxiliary which already exists.
IMHO, this is rather confusing.
It seems better to either modify the aux with an index or make some more
descriptive name.
|
|
I think once we have several instances of the same quantity (energy) getter
obtained with different methods, there are two options (unless we're going to assign long self-explanatory names):
(1) numerical convention: aux1Energy/aux2/aux3 (as you've suggested)
(2) naming convention: aux(iliary)/extra/add(ed)/suppl(ementary)/addend(um)
Now looking back into the history of changes:
Igor has added "raw" (M0) in 2014, "eaux" (M3) in 2015. So, we've moved along the option (2).
In any of the two cases we need a descriptive info behind any of those. Can
be just comments in the header.
I mean for analyst/user of these getters.
E.g. there are (all cars :) Peugeot 108/208/308 and Seat Mii/Ibiza/Leon.
…On Tue, 14 Nov 2017, Slava Krutelyov wrote:
On 11/14/17 7:43 AM, Salavat Abdullin wrote:
> Minor comment: extraEnergy (?)
> > "add" stay for additional.
both are synonyms of auxiliary which already exists.
IMHO, this is rather confusing.
It seems better to either modify the aux with an index or make some more
descriptive name.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.[AEx02gZn4oQfuYXIiQlutH3YiYDCqi-Bks5s2TqtgaJpZM4Qbv5i.gif]
|
|
On 11/14/17 8:26 AM, Salavat Abdullin wrote:
Quite some time ago I've suggested "aux", then Igor has suggested "raw", if
I remeber it right. So, we've moved along (2).
raw is actually pretty descriptive for M0 and it is an example of
incidentally short but self-explanatory names.
|
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
|
+1 |
1 similar comment
|
+1 |
|
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) |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
+1 |
Changes are only code cleanup:
removed the HBHE local reco (M2/M3/noise cleaning) from the old run1 code since now all moved to the Phase1code.
No effect is expected in any workflow
@igv4321 @kpedro88 @abdoulline