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] Pattern Recognition as Plugins, with CLUE3D addition. #33645

Merged
merged 35 commits into from May 27, 2021

Conversation

rovere
Copy link
Contributor

@rovere rovere commented May 6, 2021

PR description:

This PR will move the current Pattern Recognition algorithms in HGCAL to become plugins.
In addition to that, a new Pattern Recognition algorithm, CLUE3D, has been added, as a plugin. For the time being, it's not run in production, but it has been tested.
Pattern Recognition parameters have been moved directly into the corresponding plugins.

Expect no regression.

PR validation:

Typical single-particle workflows related to HGCAL. Tested enabling and disabling CLUE3D.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33645/22521

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

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

It involves the following packages:

RecoHGCal/TICL

@perrotta, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata can you please review it and eventually sign? Thanks.
@felicepantaleo, @apsallid, @sobhatta, @lecriste, @hatakeyamak, @ebrondol, @clelange this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@rovere
Copy link
Contributor Author

rovere commented May 6, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b59406/14910/summary.html
COMMIT: 2e3959e
CMSSW: CMSSW_12_0_X_2021-05-05-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33645/14910/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1267 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2662646
  • DQMHistoTests: Total failures: 3673
  • DQMHistoTests: Total nulls: 19
  • DQMHistoTests: Total successes: 2658932
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -45.703 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@@ -92,9 +90,17 @@ TrackstersProducer::TrackstersProducer(const edm::ParameterSet& ps, const Tracks
consumes<std::vector<TICLSeedingRegion>>(ps.getParameter<edm::InputTag>("seeding_regions"))),
itername_(ps.getParameter<std::string>("itername")) {
if (doNose_) {
auto plugin = ps.getParameter<std::string>("patternRecognitionBy");
Copy link
Contributor

Choose a reason for hiding this comment

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

This and next line can be moved out of the if, removing their repetiion in the else.


# MULTICLUSTERS

ticlMultiClustersFromTrackstersCLUE3DHigh = _multiClustersFromTrackstersProducer.clone(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want to produce these MultiClusters?


# MULTICLUSTERS

ticlMultiClustersFromTrackstersCLUE3DLow = _multiClustersFromTrackstersProducer.clone(
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, do we still want to produce these MultiClusters?

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33645/22530

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

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

@cmsbuild
Copy link
Contributor

Pull request #33645 was updated. @perrotta, @silviodonato, @franzoni, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please check and sign again.

@rovere
Copy link
Contributor Author

rovere commented May 21, 2021

@cmsbuild please test

@slava77
Copy link
Contributor

slava77 commented May 21, 2021

Pull request #33645 was updated.

I was just starting with the follow-up review.

@rovere
please clarify if you are done with updates or if something is still pending.
Thank you.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b59406/15248/summary.html
COMMIT: d501c9e
CMSSW: CMSSW_12_0_X_2021-05-21-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33645/15248/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 7 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2650486
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2650451
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@rovere
Copy link
Contributor Author

rovere commented May 21, 2021

@slava77
Done, thanks.

@rovere
Copy link
Contributor Author

rovere commented May 26, 2021

Ciao,
is there any hope this PR would enter pre2?

@slava77
Copy link
Contributor

slava77 commented May 27, 2021

+reconstruction

for #33645 d501c9e

  • code changes are in line with the PR description and reasonably follow the code review
  • jenkins tests pass and comparisons with the baseline show no (relevant) differences as expected for the part of the PR where the currently running code is refactored/reorganized in a mostly technical way
  • a local test with wf 34834.999 (PU50 phase-2) and clue3D process modifier (also with relevant keep for the output tracksters) show that the High and Low variants of CLUE3D run to completion with somewhat reasonable but perhaps still to be tuned/reduced time of 220-230 ms/evt each (vs e.g. 107 ms/evt in ticlTrackstersEM)

@srimanob
Copy link
Contributor

+Upgrade

This PR is to organize the PR algorithms to become plugin, and introduce new CLUE3D PR. No change is expected on wf as of now.

@silviodonato
Copy link
Contributor

+1

@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 be automatically merged.

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

6 participants