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

Adding HGCal configuration in electron MC Validations #20934

Merged
merged 2 commits into from Nov 3, 2017
Merged

Adding HGCal configuration in electron MC Validations #20934

merged 2 commits into from Nov 3, 2017

Conversation

archiron
Copy link
Contributor

A few modifications have been made for HGCal Phase2 integration into electron MC Validations.

2 small modifcations into Validation/RecoEgamma/plugins/ ('ElectronMcFakeValidator.cc & ElectronMcSignalValidator.cc) related to the scale limits of one histo.
there is modifications into the _cfi files to take HGcal Pahse2 modifications into account (ElectronMcFakeValidator_gedGsfElectrons_cfi.py, ElectronMcSignalValidatorPt1000_gedGsfElectrons_cfi.py & ElectronMcSignalValidator_gedGsfElectrons_cfi.py).

The config files (_cfg.py files) that we use have been rewritten to take modifications into account :
ElectronMcFakePostValidation_cfg.py, ElectronMcFakeValidation_gedGsfElectrons_cfg.py,
ElectronMcSignalPostValidationMiniAOD_cfg.py, ElectronMcSignalValidationMiniAOD_cfg.py,
ElectronMcSignalPostValidationPt1000_cfg.py, ElectronMcSignalValidationPt1000_gedGsfElectrons_cfg.py,
ElectronMcSignalPostValidation_cfg.py, ElectronMcSignalValidation_gedGsfElectrons_cfg.py

We have added 3 files for run simplification :
relval_hgcal.tcsh -> used to launch local computations & web page histos
OvalFile.HGCAL -> used to create web pages histos
electronValidationCheck_Env.py -> used for RECO/HGCal initialization. It is a new file wich is called by our config (_cfg.py) files when we create histograms web pages.

Tests have been made with runTheMatrix without any encountered pbms.

@beaudett @fcouderc @rovere

#CMSSW_9_3_X

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20934/1481

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20934/1481/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20934/1481/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20934/1485

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @archiron (Chiron) for master.

It involves the following packages:

Validation/RecoEgamma

@kmaeshima, @cmsbuild, @vanbesien, @vazzolini, @dmitrijus can you please review it and eventually sign? Thanks.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@rovere
Copy link
Contributor

rovere commented Oct 23, 2017

Hi all,
do we really need 6+ days to even trigger the tests? It seems too much to me.
@archiron Quickly looking into the code it seems that you switched the monitoring of gedElectrons with electronsFromMultiCl, which is correct since it's the only working collection we have. I was wondering if it could be better to add a cloned version of the usual validation sequence instead of replacing the inputTag, so that the moment we will have working gedGsfElectrons no changes will be needed. Let me know what you think.

@beaudett
Copy link
Contributor

Hi Marco,
may I ask you more information on the change you propose ? Are you saying that within the if (phase2) switch, the validation sequence is cloned or are you proposing to introduce a new specific config file with the Phase2 validation sequence ?
Probably, I am missing something, but as far I understand, when we switch to default gedGsfElectrons for Phase 2, we will have to change an InputTag somewhere, no ? Does your proposal go in the direction of automatically catching the change ? How ? Thanks.


from Configuration.Eras.Modifier_phase2_hgcal_cff import phase2_hgcal
phase2_hgcal.toModify(
electronMcFakeValidator,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beaudett
here we switch from gedGsfElectrons to ecalDrivenGsfElectronsFromMultiCl et all for all related collection.
What I was proposing was to clone the original analyzer and run them both at the same time so that gedGsfElectrons are left as they are (broken for the time being) and FromMultiCl added. The moment we fix the gedGsfElectrons there's nothing else/more to touch here. Would that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It would be indeed convenient. But we will have duplicate set of histograms then, one broken (with empty histograms) and one filled. Aren't the broken ones take disk-space for nothing ? If you tell me that it is not an issue, then I buy your solution !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beaudett they will take up both memory and disk space: the latter is very limited, the formes is somehow quite expensive these days. Just to have a rough understanding, of how much histograms are we talking about? I'd then put the histograms in 2 separate folders. That could also be useful to have a comparison later on between the 2 collections.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Marco,

we are speaking of > 1K histograms, with a good fraction of them being 2D.
ElectronMcFakeValidator : 360 histos
ElectronMcSignalValidator : 494 histos
ElectronMcSignalValidatorPt1000 : 494 histos
ElectronMcSignalValidatorMiniAOD : 41 histos

Arnaud reminded me that we had been asked in the past to reduce the number of histograms, and we had to struggle a bit when we introduced the Pt1000 duplicated directory (which is not empty) and for which the ranges of the histograms are adapted.
Therefore, and after some thinking, we are a bit reluctant to clone the sequence and to put a new directory in place. We are aware that it implies that we manually switch back to the regular collection once it is in place.
We hope it is agreeable to you.

Florian & Arnaud

@rovere
Copy link
Contributor

rovere commented Oct 23, 2017 via email

@beaudett
Copy link
Contributor

Hello
have the test been triggered in the end ?
Thanks F.

@rovere
Copy link
Contributor

rovere commented Oct 25, 2017

I'm begging for this but, apparently, I have no luck.

@beaudett
Copy link
Contributor

I now understand why CMS is so well ranked by github on the amount of discussions :-)

@slava77
Copy link
Contributor

slava77 commented Oct 25, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 25, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23981/console Started: 2017/10/25 18:07

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@kpedro88
Copy link
Contributor

@slava77 I also find this quite puzzling. Modifications of the Validation subsystem should not be able to affect reco. In the 2023D19 workflow, the timing-related changes are enabled - maybe this problem is related to #20621? @lgray @bendavid

@slava77
Copy link
Contributor

slava77 commented Oct 26, 2017 via email

@smuzaffar smuzaffar modified the milestones: CMSSW_9_4_X, CMSSW_10_0_X Oct 26, 2017
@cmsbuild cmsbuild modified the milestones: CMSSW_10_0_X, CMSSW_9_4_X Oct 26, 2017
@kpedro88
Copy link
Contributor

@slava77 I'll run valgrind...

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 26, 2017 via email

@kpedro88
Copy link
Contributor

@davidlange6 I had noticed the irreproducibility in Phase1 pixel validation, but I'm not sure if it is related to what was observed here (changes in handful of reco validation plots only in 20434.0, 2023D19 workflow).

@slava77
Copy link
Contributor

slava77 commented Oct 26, 2017 via email

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 26, 2017 via email

@kpedro88
Copy link
Contributor

@slava77 @davidlange6 see #21036

@dmitrijus
Copy link
Contributor

+1

@kpedro88
Copy link
Contributor

kpedro88 commented Nov 2, 2017

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2017

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 4b1d1d6 into cms-sw:master Nov 3, 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.

None yet

9 participants