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: 1st version of hadronic reconstruction #27819

Merged
merged 10 commits into from
Sep 12, 2019

Conversation

gouskos
Copy link
Contributor

@gouskos gouskos commented Aug 22, 2019

PR description:

  • add the first version of the hadronic shower reconstruction. NB. Parameters need further tuning [work in progress]
  • Introduce the "Out-In" step for the Trackster creation. Currently enabled by default for both EM and HAD reconstruction. Compatibility angles between Layer clusters can be tuned separately for the "In-Out" and "Out-In" steps.
  • Use at least 2 cells to form a Layer cluster in the Silicon part [more robust against noise, less combinatorial background]. For the Scintillator part the requirement is relaxed to 1 cell

PR validation:

First validation of the changes can be found in the following links:

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@felicepantaleo
Copy link
Contributor

@rovere @amartelli fyi

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27819/11541

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

@slava77
Copy link
Contributor

slava77 commented Aug 22, 2019

@gouskos
please note that you have to resolve issues with the code in #27819 (comment)
before we can start the compile and run time tests

@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/cms-sw-PR-27819/11543

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gouskos for master.

It involves the following packages:

RecoHGCal/TICL

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @clelange, @hatakeyamak this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@gouskos
Copy link
Contributor Author

gouskos commented Aug 22, 2019

@gouskos
please note that you have to resolve issues with the code in #27819 (comment)
before we can start the compile and run time tests

@slava77 Fixed now - sorry

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 22, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2115/console Started: 2019/08/22 16:19

@clelange
Copy link
Contributor

@steggema FYI

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #27819 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

@felicepantaleo
Copy link
Contributor

please test workflow 20493.52

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 11, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2479/console Started: 2019/09/11 11:40

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2957224
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2956882
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2019

the last commits changed mostly just the Trk clusters
Overall, since the last reco signoff for d58fa18 (to update #27819 (comment)) the Trk now look like
all_sign1087-pass-1dc050c-pu200-in2-ticlVSsign1087-pu200-in2-ticl_TTbar14TeV2026D41PUwf20634p0c_log10recoHGCalMultiClusters_multiClustersFromTrackstersTrk_TrkMultiClustersFromTracksterByCA_RECO_obj_energy

this is just to note the change in outputs.
The change in the code was already confirmed in #27819 (comment)

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2019

+1

for #27819 1dc050c

@kpedro88
Copy link
Contributor

+upgrade

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2019

the CPU time in TICL since #27819 (comment) (coming from the eta range fixes) increased almost twofold from 4.6 s to 8.3 s, which is still small compared to the rest of what's happening in PU200 reco.

@rovere
Copy link
Contributor

rovere commented Sep 12, 2019

@slava77 As I mentioned in a previous comment, the current timing could be easily reduced by half by doing proper masking (well, that will add the inference time into the game ;) ).

@slava77
Copy link
Contributor

slava77 commented Sep 12, 2019

@fabiocos
please check this, in hope that it can make it to pre8 as well.
Thank you.

@fabiocos
Copy link
Contributor

@slava77 I've already noted it for integration this evening, thanks for warning me

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 7a0e60c into cms-sw:master Sep 12, 2019
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.

10 participants