Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dead ES regions with correct energy each plane #17219

Merged
merged 1 commit into from
Apr 5, 2017

Conversation

argiro
Copy link
Contributor

@argiro argiro commented Jan 20, 2017

same as #17145 for 80x

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @argiro for CMSSW_8_0_X.

It involves the following packages:

RecoParticleFlow/PFClusterTools

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray, @rafaellopesdesa, @bachtis, @cbernet this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Jan 20, 2017

@argiro
this change technically affects the standard workflow; although practically they are protected by the values in current ESEEIntercalibConstants (while e.g. ESEEIntercalibConstants_LG_2016 will change that).
Please clarify if dead ES channels are included in the current MC campaign.

If there is a plan to change ESEEIntercalibConstants to ESEEIntercalibConstants_LG_2016 for the legacy rereco, this PR may have to wait until shortly before the legacy rereco release is built

@franzoni @arunhep @mmusich

@slava77
Copy link
Contributor

slava77 commented Jan 22, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 22, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17369/console Started: 2017/01/22 04:16

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@argiro
Copy link
Contributor Author

argiro commented Jan 22, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jan 22, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jan 22, 2017

backport of #17145

@slava77
Copy link
Contributor

slava77 commented Jan 22, 2017

+1

for #17219 9cb1c72

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017 via email

@davidlange6
Copy link
Contributor

davidlange6 commented Jan 31, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2017

From the discussion in the ORP I understood that before this can be integrated in 80X, the 90X should have the new ES calibration payload included in tests (standard release GT).

@argiro
Copy link
Contributor Author

argiro commented Feb 1, 2017

a payload is under preparation

@franzoni
Copy link

franzoni commented Feb 2, 2017

during the discussion at the alca meeting yesterday
( slide 5 of https://indico.cern.ch/event/592613/contributions/2448614/ )
we agreed with @shervin86 and ming that they would follow up clarifying what needs to be done at GT level (email thread preferable to this chat, if you may)

How were the changes already deployed in 90x, w/o the corresponding change in GT ?
@arunhep may want to follow this one too.

@slava77
Copy link
Contributor

slava77 commented Feb 14, 2017

@argiro @franzoni @shervin86
please clarify on the status of the payloads for 80X and for 90X

@cmkuo
Copy link
Contributor

cmkuo commented Feb 14, 2017

there was a confusion between me and the ECAL tag creator so the previous validation failed.
We sorted out the problem and a new tag was created. Shervin is producing ntuples for validation.
We expect the validation to be done in 1-2 days. Then we can give you the tag for 90X and also 80X if the fix is included.

@cmkuo
Copy link
Contributor

cmkuo commented Feb 16, 2017

@slava77 I would like to know whether we need a fully validated GT in order to include this fix for 90X. Or a partially validated GT which will not break the SW is OK to allow this fix to be included in 90X ?

@slava77
Copy link
Contributor

slava77 commented Feb 16, 2017 via email

@cmkuo
Copy link
Contributor

cmkuo commented Feb 18, 2017

@slava77 @shervin86 @franzoni @argiro

ESEEIntercalibConstants_Legacy2016_v3 with the payload for ES dead region calibration is fully validated.

The attached plots show the validation of one of runs in 2016D which had hardware problem to cause a dead region in the first ES plane.

Color codes : green -> MC, blue -> current tag, red -> ESEEIntercalibConstants_Legacy2016_v3

Feb_DoubleEG_3cat_ES2016D_ESEnP1_Dead.png shows the ES energy on the first plane.
You can see before the dead region calibration (blue), the ES energy does not agree with MC.
You may ask why the ES energy is not exactly zero. First of all, the x-axis does not start from 0.
Secondly, if the electrons hit the border of the dead region, the clustering can pick up the energy from the vicinity of active sensors. After the dead region calibration, the ES energy on the first plane agrees with MC.

Feb_DoubleEG_3cat_ES2016D_ESEn_Dead shows the sum of the energy for two ES planes.
You can see before the dead region calibration (blue), the energy comes from the second (active) plane. After the dead region calibration, the total ES energy agrees with MC.

feb_doubleeg_3cat_es2016d_esenp1_dead
feb_doubleeg_3cat_es2016d_esen_dead

@slava77
Copy link
Contributor

slava77 commented Feb 18, 2017 via email

@cmkuo
Copy link
Contributor

cmkuo commented Feb 18, 2017

@slava77 What do you mean make it to 900pre5 ? Do you mean to include this tag to GT for 900pre5 ?
I am in contact with Pierre Depasse to have this tag in 90X. Whether this tag can be included in GT for 900pre5, I think it's on the hand of @franzoni and Arun

@slava77
Copy link
Contributor

slava77 commented Feb 18, 2017 via email

@franzoni
Copy link

thanks @cmkuo for the plots,
stunning improvements !

How does the required update to conditions interplay with the code changes of this PR and the (already-merged) 90x (#17145) ?
That remains unclear to me. May you please explain ?
Is this PR and its 90x counterpart #17145 de facto inconsistent, and needing the an update to conditions, in order to be as good as the plots you show ?

The procedure to update a tag in GT is described here: https://twiki.cern.ch/twiki/bin/viewauth/CMS/AlCaDB#Policy_about_updates_in_the_GTs - @depasse is familiar with it all.
Integration in 90x can happen at the next PR by AlCa, which I am planning to use primarily for the morphing of the 2017 MC into HCAL plan1 (and could pick up the ES tag).

When you announce the tag, can you please explain, in comparison to the tag used in the offline queues up to now:

  • the change in IOV sctructure, where an additional IOV is added (is that the run interval with the ES inefficiencies) with since 272760
  • for the common IOV's that follow the newly introduced one, the payloads change; is that the result of the re-measurement of legacy ? If what was the case, we would need to discuss how to handle those payloads: we are purposefully not deploying any legacy-related condition updates in 90x/80x quite yet, we'll do once everything is validated. Please help us understand what the chances you propose are, we'll find a way of complementing this PR (and the 90x counterpart dead ES regions with corrected energy for each plane. Fixes inconsistency in regression training #17145 ) once we know that's cleared

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2017 via email

@franzoni
Copy link

franzoni commented Feb 19, 2017

Thanks @slava77 , helpful clarifications.

@cmkuo @depasse
I've learnt there's 1-2 days before the HCAL plan1 ingredients will become available in 90x for integration (I won't want to have other items to integrate together with HCAL plan1, since it'll be complex and priority for 900_pre5 as well as MWGR2).
If you can address the questions above and you can queue ESEEIntercalibConstants_Legacy2016_v3 where you propose to deploy it, we can exploit such time window to get it in autoCond.py

@cmkuo
Copy link
Contributor

cmkuo commented Feb 19, 2017

@franzoni

I would like to add something on top of what Slava said.
I would like to have this fix in legacy 80X CMSSW to cure the dead ES region.
If this fix is back ported to 80X for legacy re-reco, this tag will be the one for legacy re-reco for EE-ES inter-calibration.

ok, we will add the explanation when the tag is announced.
But anyway let me explain it here first.

In this new tag, there are three IOVs for 2016 data and include not only the dead region calibration but also the data/MC discrepancy for 2016 G and H as I presented in AlCa meeting on Feb 1st.
The first IOV covers 2016 B-F.
The second IOV is for 2016G.
The third IOV is for 2016H.
This tag should be used together with ESChannelStatus_V03_offline and ESIntercalibConstants_Run1_Run2_V07_offline.
So these are the tags for legacy re-reco.

If you do not want to deploy the legacy-related condition updates in 90X/80X now,
we need @depasse to take the current ESEEIntercalibConstants in 90X and just
change the payloads for the dead ES region calibration. In this case, the IOV structure
will not be changed.

@depasse
Copy link
Contributor

depasse commented Feb 19, 2017 via email

@cmkuo
Copy link
Contributor

cmkuo commented Feb 20, 2017

@depasse since @franzoni (aka ALCA) does not want to deploy legacy re-reco to 80X/90X now, we need to do a minimum payload update. In this case, we should only update the values relevant to dead region calibration. @franzoni please let us know if you want such tag so that I can tell @depasse what to change.

@cmkuo
Copy link
Contributor

cmkuo commented Feb 21, 2017

@franzoni can you please take a look at the previous comment ?

@franzoni
Copy link

@cmkuo
taken note.

Thanks for the explanations.

For 90x, to start with:
are these two

  • ESChannelStatus_V03_offline
  • ESIntercalibConstants_Run1_Run2_V07_offline

the only updates needed to deploy the dead plane behaviour and guarantee consistency? What role does ESEEIntercalibConstants_LG_offline_data_default play ?

@cmkuo maybe we can have a brief skype, to clear up the details ?

@cmkuo
Copy link
Contributor

cmkuo commented Feb 22, 2017

@slava77 @depasse @argiro @franzoni @shervin86
I just had a Skype chat with Giovanni and Arun. The summary is as follows.

  1. We need ESChannelStatus to trigger the dead region calibration. The ESChannelStatus tag (ESChannelStatus_V03_offline) with the channel status history in 2016 was already deployed in 2016
  2. However, the dead region calibration has never been in reconstruction even though the ESChannelStatus is up-to-date because the software is not updated and also the calibration constants in ESEEIntercalibConstants are set to 1 which is not correct
  3. ESEEIntercalibConstants_Legacy2016_v3 includes the correct constants for the dead ES region calibration, but also the constants for the active region for legacy re-reco
  4. ALCA convenors agreed to deploy ESEEIntercalibConstants_Legacy2016_v3 although it kind of violates their policy. However, if we try to remove legacy re-reco constants and make a new tag, it will take time to produce a new tag and do another validation, and may generate confusion among different parties.
  5. ESIntercalibConstants_Run1_Run2_V07_offline will not be deployed for 90X for the time being because it contains the constants for legacy re-reco and whether we include it or not will not affect the dead region calibration. In addition, as soon as all legacy conditions are accepted by ALCA in the near future, it will be embraced to 90X and 80X.

@franzoni
Copy link

Thanks @cmkuo
We'll integrate in 90x after the plan1 hcal will be done

@cmkuo
Copy link
Contributor

cmkuo commented Apr 5, 2017

@slava77 will you include this fix to the 80X CMSSW for legacy re-reco soon ?

@slava77
Copy link
Contributor

slava77 commented Apr 5, 2017 via email

@davidlange6 davidlange6 merged commit d889094 into cms-sw:CMSSW_8_0_X Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants