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

HGCAL end of life DIGITISATION update #30422

Merged
merged 39 commits into from Jun 29, 2020

Conversation

franzoni
Copy link

PR description:

We've documented in detail the content of this PR in yesterday's HGCAL DPG meeting https://indico.cern.ch/event/931566/

1 . Electronics specifications updated from HGCROCV2 to V3
2 . Leakage of amplitude from BX-1 updated and made gain specific
3 . fluence updated to latest maps from bril
4 . logic to detect optimal HGCROCv3 gain slightly modified to favor the MIP peak in the [8-15] ADC range

All these changes won't affect the HGCAL DIGI in startup.
They all affect only DIGI the end-of-life configuration ( SLHCUpgradeSimulations/Configuration/aging.customise_aging_3000 ).

PR validation:

We've run workflow 23434 w/ and w/o customisation for ageing at 3000/fg. No significant changes in the startup. CPU time per event decreases in the 3000/fb scenario by a few seconds, driven by the reduction in hit occupancy

pfs and others added 29 commits June 18, 2020 15:12
  . deprecated radiiVec to give more freedom for different parameterizations (needed for the latest fluka map)
- HGcalSciNoiseMap and HGCHEbackSignalScalerAnalyzer
  . propagation of the depcreation of radiiVec
- HGCalSiNoiseMap
  . adding method to get Si operation characteristics independently of the DetId
  . adding the split of the noise components
   . propagating deprecation of radiiVec
 . removing HGCROCv2 backward compatibility
 . introducing a cache to avoild re-evaluating radius, fluence and all the parameters for Si
  - added a core struct with the minimal set of variables used in the digitizer
  - added method to use the cache based on +z DetIds
. HGCSiNoiseMapAnalyzer and HGCDigitizerBase
  - propagated the change in api introduced above
  . introducing gain-dependent pulse shapes according to tables from D. Thienpont
  . adding two additional bits to dose algo to disable gain-dependent pulse shape and usage of cache

- HGCFEElectronics

  . adding method to retrieve default ADC pulse shape configured via phyton
  . adding optional adcPulse argument to run shaper methods

- HGCDigitizerBase

  . propagation of the changes above
  setGeometry method now takes care of filling the operation cache
  cache filled from the list of valid DetIds from geometry insted of dynamically from DetIds received.
  This avoids having to query if DetId is already existing in the cache.
  Given there is no need to have a fast query on the cache, switched from unordered_map to a standard map
  to reduce memory footprint

- HGCDigitizerBase
  propagation of the changes above
- disable cached op by default
   . update pad area for latest sensor specs
. update surface of pads
. modify logic of auto-gain
@franzoni
Copy link
Author

please test

@franzoni
Copy link
Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 26, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

+1
Tested at: 1f3b6f9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-716ddc/7446/summary.html
CMSSW: CMSSW_11_2_X_2020-06-26-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2778915
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2778864
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 152 log files, 16 edm output root files, 36 DQM output files

@civanch
Copy link
Contributor

civanch commented Jun 27, 2020

+1

@franzoni , I agree with Kevin, that this type of data should not be inside sub-directory but is an external data (but definition - it come our of external source and has non-negligible size).

@franzoni
Copy link
Author

hello @kpedro88 @civanch , what is the policy for files to go to external ? If appears a little bit of an overkill to us, it's a few kB.

However, if the policy is this file ought to go to external, we will do that. In which case,
could you please remind us of how to do it, perhaps there are instructions ?

It would be very helpful for us if this PR could make pre2.
Can we work on moving to the external after this PR ? It would be great if this PR could arrived signed at tomorrow's ORP...

@rovere
Copy link
Contributor

rovere commented Jun 29, 2020

9kB is definitely not a huge penalty in terms of space. Space that, in any case, we have to pay through the externals anyway. So, I'm really questioning what the added value is to have it in a separate repository?
Given the fact that this PR is on the critical path to perform detailed back-end related studies, HGCAL DPG kindly asks to push it through as is in release ASAP. Any move to external repository or, even better, to real conditions, will be kept on our todo list and not forgotten.

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 29, 2020

@franzoni @rovere keeping data files in cms-data repositories makes it easier to clone the cmssw repository for development (otherwise the repository size can grow out of control)
but in this case it's not crucial

@kpedro88
Copy link
Contributor

+upgrade

@cmsbuild
Copy link
Contributor

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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 499b240 into cms-sw:master Jun 29, 2020
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

7 participants