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

Add MultiCluster derived electrons and photons #20045

Merged
merged 36 commits into from Aug 15, 2017
Merged

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Aug 3, 2017

This PR will introduce new collections for electrons and photons derived from HGCal multiclusters.

More details could be fetched from here: link

I expect no regression at all. The affected workflows are the ones using HGCal-PhaseII era.

I also profited from the existing EGamma-POG DQM to monitor the newly created collections. Yet, minimal changes had to be introduced also there, but the final output for regular (i.e. non-upgrade) workflows should be identical.

@felicepantaleo @malgeri @cseez @kpedro88

rovere added 25 commits July 26, 2017 14:46
For ecalDrivenGsfElectronCoresFromMC we need to use the gsfTracks
track collection in order to make Ref for electrons in the final
electrons collection, since this is the only collection that has been
directly produced consuming the HGCal Multi Clusters. Failing to do
so, we would end up using the PFTrack collection that, instead, is
using the (realistic) simCluster information.
Define PhotonProducerHGCal as a Framework module and instruct the
underlying python configuration to use it. This last point has been
achieved in a rather hacking fashion that, indeed, proved to work and
saved quite a few configuration duplication.
HGCal photons do not have regular isolation variables
defined. A simpler selection is made using only Et and eta
and pretend those photons are "isolated". This simplified
selection is only activated in the HGCal phaseII scenario.
Also, the invariant mass plot has bin properly partitioned
in barrel-barrel, endcap-endcap and endcap-barrel. This is
true for all cases, not only for the HGCal Phase era.
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20045/90

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20045/90/git-diff.patch

In future, you can run scram build code-checks to apply code checks

@slava77
Copy link
Contributor

slava77 commented Aug 13, 2017

@slava77
Copy link
Contributor

slava77 commented Aug 13, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 13, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22268/console Started: 2017/08/13 19:59

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Sam-Harper
Copy link
Contributor

Likewise I have no real comments, the changes to the E/gamma general code look good. Thanks for all this!

If I was being picky, I would have liked a comment here to make it easier to understand what was going on here (it has terms like "ecal" when its not ecal, etc so while understandable, it would be made faster to understand with a comment)
https://github.com/rovere/cmssw/blob/e7def2cc32ccebb1bd007d5e89f855a4700ae74b/RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc#L698-L710

But I'm okay with this not happening, given we want this in and done :)

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 363 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2713411
  • DQMHistoTests: Total failures: 301
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2712921
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@dmitrijus
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

the last update introduced changes in ootPhotons in HI reco: the number of passing photons reduced. The ootPhotonCore in HI has 8 GeV threshold while the ootPhotons have 10 GeV. Due to the effectively missing cut before the last change we had more ootPhotons. Now the cut is applied as expected from the configuration.

@rovere
Copy link
Contributor Author

rovere commented Aug 14, 2017 via email

@rovere
Copy link
Contributor Author

rovere commented Aug 14, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

On technical performance based on tests in 930pre3 (signal and PU re-generated) with PU settings 'AVE_200_BX_25ns,{"B":(-3,3)}' using 100 events in workflow 20234.2:

  • new products take about 4.9 MB/event , which is about 15% of the RECOSIM 33 MB/event total
    • the set of products is the same as in Add MultiCluster derived electrons and photons #20045 (comment)
    • I see 60% larger size increase estimates than Marco, but maybe that is due to the release difference. relvals will give a proper estimate for production needs. Anyways, there is no intent of fine-tuning the selections to change the increment.
  • CPU increase from added modules is 25 s/event, which is close to 10% in workflows without timing (total reco time from a sum of TimeModule in my test was 450s of which 200s was in the timing-related modules), in agreement with Add MultiCluster derived electrons and photons #20045 (comment)
  • there was no significant increase in memory use: the peak of about 17GB in the test job with 4 cores has increased by only 4 MB.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

@rovere
I treat the change in ootPhotons as an expected change.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

+1

for #20045 e7def2c

  • replicated a large fraction of egamma reco to use actual reco clusters by adding modules *FromMultiCl; this is meant to be somewhat temporary until the sim truth based reco can be abandoned in phase-2 HGCAL reco
    • The PR also includes a move of the legacy photon reco from PhotonProducer to GEDPhotonProducer in all workflows, which now gives photons filled more consistently with the gedPhotons
  • jenkins tests pass and comparisons with baseline show differences in expected places:
    • in most workflows in photon shower shape variables
  • local tests in phase-2 with 20234.2 with PU200 show roughly expected behavior: new products are filled Add MultiCluster derived electrons and photons #20045 (comment), existing products are not affected

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 512d77c into cms-sw:master Aug 15, 2017
@kpedro88
Copy link
Contributor

@slava77 for future reference, -3,+3 is the default for AVE_200_BX_25ns now:
https://github.com/cms-sw/cmssw/blob/CMSSW_9_3_X/Configuration/StandardSequences/python/Mixing.py#L152

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