Ported 80X regression in 94X #22410
Ported 80X regression in 94X #22410
Conversation
|
A new Pull Request was created by @jainshilpi for CMSSW_9_4_X. It involves the following packages: RecoEgamma/EgammaTools @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here
|
|
we need a PR for the master branch first. Please clarify if this PR changes anything in default miniAOD or AOD. |
|
@slava77 I will do this for master branch. This does not change anything in default |
|
|
If this is needed only for analysis, when is the release needed with this feature? |
|
hi @slava77 is this V1 regression going to be used in 80XLegacy re-miniAOD? If this is needed only for analysis, when is the release needed with this feature? Next, I am going to create a pull request for 10X. thanks |
|
Hi Shilpi
On 3/2/18 3:04 AM, jainshilpi wrote:
hi @slava77 <https://github.com/slava77>
if this is for analysis of the Moriond 2017 miniAOD, can the analyzers
simply run in 80X?
===== yes they can simply run in 80X. But if they need to run in 94X
(e.g. Htogg people were trying to run 80X samples in 94X series as they
wanted to use core framework features for advanced MVA's which were only
available in 9X) then this would be needed.
Are they reading 80X AOD in 94X or are they reading miniAOD made in 80X
using 94X release?
@arizzi @gpetruc
please remind me if reading of miniAOD made in 80X still makes sense in
94X from the data format compatibility point of view.
is this V1 regression going to be used in 80XLegacy re-miniAOD?
===== We would like to stay with V1 regression for 80XLegacy re-miniAOD
Please point me to some plots confirming that V1 works better than V2.
I can imagine that the differences in PF thresholds can matter, but for
most egamma objects we are in the high interest in both 2017 and 2018.
So, what would remain is the transparency correction and slight increase
in electronics noise. I'm curious to see that it has a significant impact.
…
If this is needed only for analysis, when is the release needed with
this feature?
==== I think no one has expressed need for this feature except for the
Htogg people. But would be good to have it in the next release if possible.
Next, I am going to create a pull request for 10X.
|
|
94X can read 80X miniaod, indeed the NANOAOD production workflow we just run on 10B Moriond17 events was doing so. |
I expect that the regression applied in 80XLegacy re-miniAOD is done with the same strategy as for 94X2017. The proposal in #22308 apparently does not align with the comment above in an obvious way. @jainshilpi @Sam-Harper |
|
hi @slava77, Let me reply to this one first: Are they reading 80X AOD in 94X or are they reading miniAOD made in 80X using 94X release? I will get back to the other ones as well. thanks, |
|
On 3/2/18 6:45 AM, jainshilpi wrote:
hi @slava77 <https://github.com/slava77>,
Let me reply to this one first: Are they reading 80X AOD in 94X or are
they reading miniAOD made in 80X using 94X release?
=== they were reading 80X miniAOD in 94X.
I will get back to the other ones as well.
Andrea's answer has already covered my question.
I knew that we can read AOD made in 80X, now I know that we can read
miniAOD as well in a consistent way.
|
|
backport of #22417 |
Please clarify on this. |
|
hi @slava77 , We would not like to support "EGExtraInfoModifier*" but a renamed version of it: EGRegressionModifierV2.cc would be supported to make it more comprehensible in future as we are going to support the old version of regression. This way, we will be able to keep track of versions of the regressions. Also, this PR concerns only porting of 80X regression into 94X, thereby I think independent of "ElectronEnergyCalibrator*". thank you |
|
On 3/5/18 7:29 AM, jainshilpi wrote:
hi @slava77 <https://github.com/slava77> ,
We would not like to support "EGExtraInfoModifier*" but a renamed
version of it: EGRegressionModifierV2.cc would be supported to make it
more comprehensible in future as we are going to support the old version
of regression. This way, we will be able to keep track of versions of
the regressions.
For "ElectronEnergyCalibrator*", I discussed with Sam. currently Sam is
trying to make something working for the analyzers. Though he plans to
support it. I let @Sam-Harper <https://github.com/sam-harper> comment on
it further.
Also, this PR concerns only porting of 80X regression into 94X, thereby
I think independent of "ElectronEnergyCalibrator*".
right, but then it looks like the plan is to ~immediately reimplement
this code in ElectronEnergyCalibrator, given that 80X reminiAOD is imminent
So, either the lifetime of this code is negligible (not worth to be
added in the release),
or there is a long term commitment to support a duplicate version with
"EGExtraInfoModifier*" and with "ElectronEnergyCalibrator*"
|
|
hi @slava77 , Making the regression and smearing run together is a future plan (@Sam-Harper can comment on it further) thank you |
|
hi @slava77 Replying to the other question: I expect that the regression applied in 80XLegacy re-miniAOD is done with the same strategy as for 94X2017. The proposal in #22308 apparently does not align with the comment above in an obvious way. ===== yes you are right and also we would like to be consistent with scale and smearing. Since scale and smearing for 2016 Legacy re-miniAOD is done on V2 regression, so we will be staying with V2 for 2016 legacy and not with V1 as I mentioned earlier. thanks |
|
it looks like there may be some interference between this PR and the code for miniAOD v2 needs. |
|
hi @slava77 I just changed the base to CMSSW_9_4_MAOD_X as you suggested. thanks, |
|
@cmsbuild please test |
|
Comparison is ready Comparison Summary:
|
|
@cmsbuild please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
@fabiocos |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_4_MAOD_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_1_X is complete. 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) |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
@slava77 as we need to make a test build anyway and then bring back to 94X, as originally planned, I see no reason not to merge it now in 9_4_MAOD_X , as it is the only leftover |
|
+1 |
This pull request ports the 80X EGM regression in 94X. It was reported by Htogg people that they faced problems in using 80X regression in 94X (i.e. loading an object that reloads database info and reevaluates information). This was because the EGM regression changed in 9X. So EGMPOG now plans to maintain the previous versions of regression as well. Changes done:
(1) Renamed : RecoEgamma/EgammaTools/plugins/EGExtraInfoModifierFromDB.cc → RecoEgamma/EgammaTools/plugins/EGRegressionModifierV2.cc
(This contains the latest version of corrections)
(2) Make a new class RecoEgamma/EgammaTools/plugins/EGRegressionModifierV1.cc → This is essentially the class which has 80X regression.
(3) RecoEgamma/EgammaTools/python/regressionModifier_cfi.py → This now contains a cms.PSet named regressionModifier80X which contains all the parameters set for 80X regression.
This setup has been tested on 80X root file in 94X and the reloading of database info and re-evaluating information is running fine now.
Thanks,
Shilpi