-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Premix option c, with 2 tags, ECAL, 11_0_X #27974
Premix option c, with 2 tags, ECAL, 11_0_X #27974
Conversation
@amassiro, CMSSW_11_0_X branch is closed for direct updates. cms-bot is going to move this PR to master branch. |
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27974/11854
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
@amassiro Please rebase this PR to master to resolve the conflicts and then apply the code-checks/code-format. |
3274397
to
12b3237
Compare
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27974/11921
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27974/11923
|
Pull request #27974 was updated. @civanch, @christopheralanwest, @mdhildreth, @cmsbuild, @franzoni, @tocheng, @tlampen, @ggovi, @pohsun can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
-1 Tested at: 260a61d You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals
When I ran the RelVals I found an error in the following workflows: runTheMatrix-results/250202.181_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25/step3_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25.log |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
and it's due to the missing: But as shown here: process.EcalLaserCorrectionServiceMC = cms.ESProducer("EcalLaserCorrectionServiceMC") In the python script. How do I add it in "run the matrix" ? |
@amassiro @fabiocos To add, you may need to go to the step PREMIXUP18_PU25 in which points to https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py#L1721-L1729 |
Hi, I'm not sure I have understood. Where can I define the new line
to be picked up automatically by "runTheMatrix"? |
Hi @amassiro Sorry for late reply, I somehow miss this. I mean you may consider to modify steps['DIGIPRMXUP17_PU25']=merge([digiPremixUp2017Defaults25ns,{'--customise_commands':'"process.EcalLaserCorrectionServiceMC = cms.ES However, this will change behavior of all 17 premixing relvals. What I proposed is to have a dedicated step, i.e. DIGIPRMXUP17_PU25_RD and we have a dedicated workflow to test. If we have a new step, then we can add from @fabiocos Could you please comment on the implementation? Thanks. |
The customization is almost ready. I am facing the following problem:
This PR requires a new object in the DB ("EcalLaserAPDPNRatiosMC"), however, being in the same format of "EcalLaserAPDPNRatios" I managed in the past to test the code by using "EcalLaserAPDPNRatios" with the following workaround:
but unfortunately this does not work anymore here. Do you have any suggestion? Should we get the new object in the db first? Is any workaround available for testing purposes? |
I think you can do |
The problem is that also with
I get the error aforementioned, while in 10_6 I was not getting it ... Do you know if anything changed on DB side? |
@ggovi @tocheng @tlampen @christopheralanwest |
Hi @amassiro (*) Wed Oct 9 00:25:38 CEST 2019 Current Modules: Module: PreMixingModule:mixData (crashed) A fatal system signal has occurred: segmentation violation |
@amassiro |
We don't have yet the object "EcalLaserAPDPNRatiosMC" in the DB, but it will have to be included later. In CMSSW_10_6_0 I was able to do the workaround of mapping the tag "EcalLaserAPDPNRatios_UL_2017_mc" into the record "EcalLaserAPDPNRatiosMCRcd". I will try to investigate further the origin of the error messages I am getting. @srimanob : I don't know why you are getting the "segmentation violation" ... I am building currently on top of CMSSW_11_0_X_2019-10-06-2300 |
@@ -172,6 +175,7 @@ REGISTER_PLUGIN(EcalIntercalibErrorsRcd, EcalCondObjectContainer<float>); | |||
REGISTER_PLUGIN(EcalADCToGeVConstantRcd, EcalADCToGeVConstant); | |||
REGISTER_PLUGIN(EcalLaserAlphasRcd, EcalCondObjectContainer<float>); | |||
REGISTER_PLUGIN(EcalLaserAPDPNRatiosRcd, EcalLaserAPDPNRatios); | |||
REGISTER_PLUGIN(EcalLaserAPDPNRatiosMCRcd, EcalLaserAPDPNRatiosMC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amassiro this line is instructing the Framework to read the payloads of the tag "EcalLaserAPDPNRatios_UL_2017_mc" as "EcalLaserAPDPNRatiosMC" objects. No way to make this working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amassiro I mean, the line in itself is fine, but you need to read payloads of type "EcalLaserAPDPNRatiosMC"
New PR, namely #28214, with all the fixes implemented cleanly (after an iteration via email, thanks db expert!) |
@amassiro so can this PR be closed, as it is superseded by the new one? |
First implementation of the ECAL run dependent MC
Similar to #27970, but that was in 10_6_X
To be tested it needs this PR: #27649
Physics details in the presentation at ECAL DPG here: https://indico.cern.ch/event/847097/
This PR will affect only the mixing step, when the premixed library is mixed with the hard scattering. The idea is that the pileup response, that has been simulated with a given transparency for ECAL crystals, will be scaled to match the transparency of the corresponding IOV.
A new object is introduced ( EcalLaserAPDPNRatiosMCRcd ), that has 1 single IOV, thus allowing this to be used as reference for scaling.
Goal: correct simulation of the photostatistics for pileup, when producing run dependent MC with several IOVs (main changes in run dependent MC for ECAL are transparency and noise).
This PR is needed for testing via relval, checking physics performance and possibly imploved data-MC comparison.