Ecal phase2 new digitization 11 2 x #31726
Conversation
|
The code-checks are being triggered in jenkins. |
|
An automatic workflow is still missing for Era_Phase2C11_Ecal_Devel, nevertheless this is a starting point for other ecal developer to upgrade the reconstruction step using the new digis. Thus they can use this era in order to have the new input for the reco step. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31726/18920
|
|
A new Pull Request was created by @dariosol for master. It involves the following packages: CalibFormats/CaloObjects @civanch, @yuanchao, @qliphy, @silviodonato, @christopheralanwest, @mdhildreth, @cmsbuild, @franzoni, @tocheng, @tlampen, @ggovi, @pohsun, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| process.load("SimCalorimetry.EcalSimProducers.esEcalLiteDTUPedestalsProducer_cfi") | ||
|
|
||
| from CondCore.CondDB.CondDB_cfi import CondDB | ||
| CondDB.connect = cms.string('sqlite_file:SimCalorimetry/EcalSimProducers/data/simPulseShapePhaseII.db') |
There was a problem hiding this comment.
@christopheralanwest @ggovi
This is in direct and flagrant violation of #27393
There was a problem hiding this comment.
Indeed. If this payload is needed, please provide a validation of the new tag and we can add it to the global tag. Can the new tag be used independent of this PR or must the code and conditions be changed simultaneously?
There was a problem hiding this comment.
Well, the code compiles and runs even without the tag, but the results are non physical because the shape of phase1 is much larger. Indeed we are trying to have this "development" branch as a temporary workflow, because the reco part must be finished. it would be nice to have the code shared with other people.
The new pulse shape is used only in ecal_devel Era, in this, let's say, sub workflow which will be used by other ecal groups. Thus, what is the best? merge without the new pulse and test the workflow (i mean reading back the new digi collections) but with a wrong shape, or it is possible to put it somewhere temporary? if not we can ask for the ALCA approval. Thanks for the advices.
There was a problem hiding this comment.
The current Phase 2 GT 112X_mcRun4_realistic_v2 contains the tag EcalSimPulseShapePhaseI, the same GT that is used in the 2018 GT. Can we use the EcalSimPulseShapeRcd payload contained in simPulseShapePhaseII.db even without this PR or would that give incorrect results? I'm not interested in merging this PR prior to updating the DB payload but rather the reverse.
There was a problem hiding this comment.
OK we will upload the payload contained in simPulseShapePhaseII.db
There was a problem hiding this comment.
the pulse shape is now on the DB, thus, as you can see here https://github.com/dariosol/cmssw/blob/b9214d31f4c48eb4765fd6ceeb1f890fd9b05422/SimCalorimetry/Configuration/python/ecalDigiSequence_cff.py#L31
we take the pulse shape as a new tag of the record EcalSimPulseShapeRcd.
Hope this is better than before.
I also move the modifier in the ecalDigiSequence_cff, to avoid modifying Digi_cff
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31726/19013
|
|
+1 |
|
@chayanit can you please sign this PR again if you are still happy with it? |
|
+1 |
| float meanarray[ecalPh2::NGAINS] = {13., 8.}; | ||
| float rmsarray[ecalPh2::NGAINS] = {2.8, 1.2}; |
There was a problem hiding this comment.
I would like the opinion of @ggovi prior to signing off for alca: should these be converted to std::vector to avoid changing the definitions of the member data if ecalPh2::NGAINS is modified at some point in the future?
There was a problem hiding this comment.
In principle the number of gains is fixed by the HW (The CATIA ASIC) so unless we change the design of the chips this should never change.
There was a problem hiding this comment.
OK, I think it's fine then.
|
+alca |
|
+operations @cms-sw/db-l2 Do you have any comments? |
|
@ggovi could you take a look at this PR please and eventually sign? |
|
+1 |
cmsbuild
added
db-approved
fully-signed
and removed
db-pending
pending-signatures
labels
|
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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
The branch contains the modifications of the Ecal Digi step to have the new digis with 160MHz sampling and 16 samples collected for phase2. The new digitizer is called by the modifier_phase2_ecal_devel, from the Era_Phase2C11_Ecal_Devel. It creates the EcalDigiCollectionPh2. It also switches off the EE and ES detectors.
It is the continuation of the PR: #29386, but we moved from 11_1_X to 11_2_X.
Most of the code have been already reviewed in the 11_1_X merge topic.
It it the baseline to start the new reconstruction for ecal phase2.
PR validation:
The test has been done using run the matrix:
Then running the full runTheMatrix.py -l limited -i all --ibeos two processes didn't finish the run:
35 34 31 23 15 4 1 1 1 tests passed, 0 0 2 0 0 0 0 0 0 failed
both with the error:
----- Begin Fatal Exception 09-Oct-2020 13:55:18 CEST-----------------------
An exception of category 'Configuration' occurred while
[0] Constructing the EventProcessor
[1] Constructing module: class=PreMixingModule label='mixData'
Exception Message:
MissingParameter: Parameter 'timeDependent' not found.
----- End Fatal Exception -------------------------------------------------
if this PR is a backport please specify the original PR and why you need to backport that PR:
It is the continuation of the PR: #29386, but we moved from 11_1_X to 11_2_X.