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

Hcal spring cleaning #14856

Merged
merged 6 commits into from Jun 20, 2016
Merged

Hcal spring cleaning #14856

merged 6 commits into from Jun 20, 2016

Conversation

kpedro88
Copy link
Contributor

(Closer to summer now, but oh well...)

A large number of unfinished and/or unused features have accumulated in the HcalDigitizer code over many years. This PR removes them to make the code easier to understand and maintain. The removal of these features was discussed on the HCAL hypernews: https://hypernews.cern.ch/HyperNews/CMS/get/hcal-cmssw/194.html

Coming in subsequent PRs:

  • removal of HcalUpgradeDataFrames (once reco for QIE10 and QIE11 is finished, to keep Phase2 local reco alive)
  • reorganization of HcalDigitizer (and maybe CaloTDigitizer) to be more object-oriented

attn: @abdoulline, @bsunanda

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X.

It involves the following packages:

SimCalorimetry/CaloSimAlgos
SimCalorimetry/HcalSimAlgos
SimCalorimetry/HcalSimProducers
SimCalorimetry/HcalTestBeam
SimGeneral/MixingModule

@cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please review it and eventually sign? Thanks.
@wmtan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@kpedro88
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 13, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13471/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14856/13471/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • 134.911_RunSinglePh2015D+RunSinglePh2015D+HLTDR2_25ns+RECODR2_25nsreHLT+HARVESTDR2

@kpedro88
Copy link
Contributor Author

Appears to be something wrong in the comparison, I'm investigating.

@abdoulline
Copy link

Juts would like to add:
from my simple pion gun scan it looks like SIM is OK, while only HE reco exists (no HB/HO/HF)
NB: Problem is observed in Digis (HE has meaningful ones, while HB/HO/HF - only pedestals/noise ones), when looking more closely at all available histos via TBrowser (and not only at those "published" below)

https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/810pre7_PR14856_vs_810pre7_SinglePi/

@kpedro88
Copy link
Contributor Author

@abdoulline - yes, I've also seen that the problem is in the digis somewhere. I plan to run git bisect to isolate the source of the problem, just haven't had time yet.

@cmsbuild
Copy link
Contributor

Pull request #14856 was updated. @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please check and sign again.

@kpedro88
Copy link
Contributor Author

@abdoulline it looks like the HO Zecotek vs Hamamatsu SiPM setting actually is used in the run1_mc GT. Do we want to keep this around to preserve run1 results? It seems like a lot of extra stuff in the SimParameters to me, but I can revert the removal if necessary.

(It also causes minor fluctuations in the HB/HE/HF results, because of fewer random number calls without the HO SipmHitResponse.)

@abdoulline
Copy link

Mea culpa... I forgot that the fact we were using different shapes Id 

specifically for HO Zekotec and Hamamatsu both in HcalMCParams and
HcalRecoParams (the latter means - for data as well with appropriately set
IOVs ...) may affect HO types removal. And intorducing some tricks in
the code to cope with it might not be an elegant option at all...

I'm not sure we can (= will be allowed to do so by AlCa)
remove this stuff from DB by making new tags for Run 1 with
"unified" SiPMs HO shapes (as we have it for Run 2), even if we know HO
didn't really matter during Run 1...

On Sun, 19 Jun 2016, Kevin Pedro wrote:

@abdoulline it looks like the HO Zecotek vs Hamamatsu SiPM setting actually is used in the run1_mc GT.
Do we want to keep this around to preserve run1 results? It seems like a lot of extra stuff in the
SimParameters to me, but I can revert the removal if necessary.

(It also causes minor fluctuations in the HB/HE/HF results, because of fewer random number calls
without the HO SipmHitResponse.)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02lgFQqMAWMiAiOqjMQkVmqsNY73mks5qNM-egaJpZM4I0eUG.gif]

@kpedro88
Copy link
Contributor Author

Ah, I hadn't considered the RecoParams, that complicates things further.

It seems unfortunate to keep a bunch of unnecessary code in HcalSimParameterMap and HcalDigitizer just to somehow preserve Run1 features that will never actually be used in 81X... but maybe we should at least push that issue to a future PR (like the few things I mentioned in the first message, as well as the possible deletion of HcalCholeskyMatrices from the database) so this one can be merged.

@abdoulline
Copy link

 Let us try to clarify with AlCa/Offline if CMS is still going to use 

81X for Run 1 MC (I presume not) or data processing (I hope not, but even
if yes, HO role was really minimal, if any even in late Run 1)...
If not, then we can remove those items (including access to
HcalCholeskyMatrices in DBService, which then would allow us to clean up
DB).

On Sun, 19 Jun 2016, Kevin Pedro wrote:

Ah, I hadn't considered the RecoParams, that complicates things further.

It seems unfortunate to keep a bunch of unnecessary code in HcalSimParameterMap and HcalDigitizer
just to somehow preserve Run1 features that will never actually be used in 81X... but maybe we
should at least push that issue to a future PR (like the few things I mentioned in the first
message, as well as the possible deletion of HcalCholeskyMatrices from the database) so this one
can be merged.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02h7J4_ku336xwo_8J12uNYr4Cu3Cks5qNOATgaJpZM4I0eUG.gif]

@kpedro88
Copy link
Contributor Author

That sounds good. I'm going to rebase the HO commit out of this PR so we can get a clean comparison, and then we can discuss with AlCa another PR that would hopefully eliminate both the unnecessary HO stuff and the HcalCholeskyMatrix DB object.

@cmsbuild
Copy link
Contributor

Pull request #14856 was updated. @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please check and sign again.

@kpedro88
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 19, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13587/console

@davidlange6
Copy link
Contributor

@kpedro88

by “Run 1”… if you mean features needed for run 1 hardware, then they are needed. If you mean features used in the simulation during 2010-2012 that are now replaced by something better (for run1 and run2 simulation) then lets try to remove them

On Jun 19, 2016, at 8:22 AM, Kevin Pedro notifications@github.com wrote:

Ah, I hadn't considered the RecoParams, that complicates things further.

It seems unfortunate to keep a bunch of unnecessary code in HcalSimParameterMap and HcalDigitizer just to somehow preserve Run1 features that will never actually be used in 81X... but maybe we should at least push that issue to a future PR (like the few things I mentioned in the first message, as well as the possible deletion of HcalCholeskyMatrices from the database) so this one can be merged.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@kpedro88
Copy link
Contributor Author

@davidlange6 my primary concern is cleaning up unused things in the simulation. For a fully accurate Run1 simulation, we would need to have these different types of SiPMs in various channels of HO... but it's not clear to me why we need or expect a fully accurate Run1 simulation in 81X and beyond. (Especially since HO was not really used for physics in Run1, and I don't think anyone expects to be able to go back to this intermediate hardware installation/development case.)

@davidlange6
Copy link
Contributor

On Jun 19, 2016, at 9:25 PM, Kevin Pedro notifications@github.com wrote:

@davidlange6 my primary concern is cleaning up unused things in the simulation. For a fully accurate Run1 simulation, we would need to have these different types of SiPMs in various channels of HO... but it's not clear to me why we need or expect a fully accurate Run1 simulation in 81X and beyond. (Especially since HO was not really used for physics in Run1, and I don't think anyone expects to be able to go back to this intermediate hardware installation/development case.)

we still support run1 in 81x… that can of course change in time, but not it hasn’t been proposed yet (just the opposite was desired a year ago, but things change)

All for cleaning up unused things!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@kpedro88
Copy link
Contributor Author

Okay, I guess we'll leave it for now. We can potentially revisit this as the HE SiPM code development continues. Some of the parameters for SiPMs currently in HcalSimParameters from Python may be put into the database.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@kpedro88
Copy link
Contributor Author

@civanch the comparison looks good to me now.

@civanch
Copy link
Contributor

civanch commented Jun 20, 2016

+1

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

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.

None yet

5 participants