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] TICL in reconstruction #29081

Merged
merged 74 commits into from Apr 9, 2020
Merged

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Mar 3, 2020

Add TICL to Phase2 Reconstruction

This is a PR for the inclusion of TICL into the standard Phase2 reconstruction.
It will run TICL by default and save its output products into the output file for several data-tiers.

The objects reconstructed by TICL are not injected into ParticleFlow by default. A customization function has been provided (injectTICLintoPF) in order to swap the sim-Assisted reconstruction in HGCAL with the one coming from TICL.

Non-optimal performances on complex events have to be expected out of the box (i.e. quite some development will have to be implemented on top of what is provided in this PR).

Results based on the development contained in this PR have been shown at a recent HGCAL DPG Meeting: link

A special thanks to @gouskos for helping during the development and testing phase.

PR validation:

The limited Matrix run w/o issues.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2020

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29081/13966

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@slava77
Copy link
Contributor

slava77 commented Mar 3, 2020

The final scram build code-format has not been run, yet: we will run it last once the PR is final.

tests can not be started without passing code-checks.
review would be a bit easier if tests can run as well; that's unless this is not expected to run yet.

@rovere
Copy link
Contributor Author

rovere commented Mar 3, 2020

The final scram build code-format has not been run, yet: we will run it last once the PR is final.

tests can not be started without passing code-checks.
review would be a bit easier if tests can run as well; that's unless this is not expected to run yet.

Thanks, @slava77 I was not aware that code-checking was a requirement to run the tests.
I'll apply and push the commit.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29081/13975

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2020

A new Pull Request was created by @rovere (Marco Rovere) for master.

It involves the following packages:

Configuration/EventContent
Configuration/StandardSequences
DataFormats/HGCalReco
RecoHGCal/Configuration
RecoHGCal/TICL
Validation/HGCalValidation

The following packages do not have a category, yet:

RecoHGCal/Configuration
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@perrotta, @andrius-k, @kmaeshima, @schneiml, @kpedro88, @cmsbuild, @silviodonato, @franzoni, @jfernan2, @fioriNTU, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @vandreev11, @lecriste, @sethzenz, @bsunanda, @makortel, @felicepantaleo, @riga, @GiacomoSguazzoni, @lgray, @cseez, @apsallid, @sobhatta, @pfs, @deguio, @hatakeyamak, @VinInn, @dgulhan, @clelange this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2020

The code-checks are being triggered in jenkins.

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2020

full reports PR vs original can be found here:
http://fpantale.web.cern.ch/fpantale/diff_eventSize_step3_29081.html
http://fpantale.web.cern.ch/fpantale/diff_eventSize_inMINIAODSIM_29081.html

Thank you @felicepantaleo

Sizes blow up at 200 PU wrt the no-PU case in #29081 (comment) ...

Do you have the total event size numbers for the two samples (before and after this PR: I don't see them in your logs), so that we can get an idea of the relative size increase in output? It was 8.6% increase overall for the no-PU case.

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2020

unhold
(I think this can be un-held @rovere , since the fix has been integrated already)

@cmsbuild cmsbuild removed the hold label Apr 8, 2020
@felicepantaleo
Copy link
Contributor

The overall step3.root file size in bytes when running wf 23306.0 on 100 ttbar + PU200 events:

original PR difference (%)
10252191486 11836149551 15.4

@rovere
Copy link
Contributor Author

rovere commented Apr 8, 2020

unhold
(I think this can be un-held @rovere , since the fix has been integrated already)

Yes, indeed, thanks @perrotta

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2020

+1

  • The new producers to allow TICL in reconstruction are implemented here
  • Those products are stored in the reco output, but not yet used in the following reconstruction or PF
  • This is an ongoing work, and further optimizations and improvements are expected. The performance in terms of timing and additional event size are however already acceptable for having these products in the Phase2 productions
    • Some slimming of the products which mostly contribute to the 15.4% overall expansion of the output size in the 200 PU case is however advisable at some point
  • Jenkisn tests pass and show no differences besides the added products (and the spurious ones reported above)

@jfernan2
Copy link
Contributor

jfernan2 commented Apr 8, 2020

+dqm

@kpedro88
Copy link
Contributor

kpedro88 commented Apr 8, 2020

+upgrade

@silviodonato
Copy link
Contributor

+operations

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

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

desc.add<edm::InputTag>("ticlCandidateSrc", edm::InputTag("ticlCandidateFromTrackstersProducer"));
descriptions.add("pfTICLProducer", desc);
desc.add<edm::InputTag>("ticlCandidateSrc", edm::InputTag("ticlCandidateFromTracksters"));
descriptions.add("pfTICL", desc);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the automatic replacement of *Producer in the product names inlcuded this one by mistake, see #29446

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