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 using FastJet #36172

Merged
merged 5 commits into from
Jan 11, 2022

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Nov 18, 2021

PR description:

A novel pattern recognition plugin has been added to the TICL framework based on FastJet.
At present, only anti-kt clustering algorithm has been added. All algorithms from the rich FastJet library could be tested using different configurations.
A processModifier has been added to allow the end-user to enable this plugin. The modifier is called fastJetTICL.
The modifier has been added in such a way that all the validation plots for the Tracksters produced by the FastJet pattern recognition will be automatically produced.
At present jets are translated into Tracksters. As a future development, if needed, we could save also the reconstructed jets.
It would be nice to have this code integrated into the release, even if not bundled to any official workflow, to allow interested people to develop on top of that.

You can find some more information about its performance (it's quite hard to call them as such, as of yet...) here.
This PR has also been recently announced during this reconstruction meeting.

PR validation:

Run on Phase2 dedicated workflows and private multiparticle samples w/o issues.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36172/26734

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • RecoHGCal/TICL (upgrade, reconstruction)

@perrotta, @cmsbuild, @AdrianoDee, @srimanob, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@missirol, @makortel, @felicepantaleo, @Martin-Grunewald, @apsallid, @sobhatta, @lecriste, @hatakeyamak, @ebrondol, @fabiocos this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@rovere
Copy link
Contributor Author

rovere commented Nov 18, 2021

assign hgcal-dpg

@cmsbuild
Copy link
Contributor

New categories assigned: hgcal-dpg

@felicepantaleo,@rovere,@pfs,@cseez you have been requested to review this Pull request/Issue and eventually sign? Thanks

@rovere
Copy link
Contributor Author

rovere commented Nov 18, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-077fa0/20616/summary.html
COMMIT: c302229
CMSSW: CMSSW_12_2_X_2021-11-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36172/20616/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: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3327156
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3327122
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Nov 23, 2021

For my understanding, is the TF model the same across these four modules?

Perhaps there is an opportunity to reduce code duplication here by a shared evaluator.

@rovere
Copy link
Contributor Author

rovere commented Nov 23, 2021

Ciao @jpata yes indeed, the model is the same, for the time being.
In parallel, we are working on the factorization that was already suggested some months ago to save memory by re-using the same session for the same model in several plugins. That will come, at some point in the near future.
In a little farther away future, depending on the use cases for the different plugins, it is quite likely we will have different models for the different plugins. In this sense, the shared evaluator will be less important, since shaping batching and everything that comes with that will likely be different.
Benedikt, as you know, is actively working on this front.

@jpata
Copy link
Contributor

jpata commented Nov 25, 2021

@rovere
Copy link
Contributor Author

rovere commented Nov 26, 2021

Ciao @jpata I took the same path we had taken for the CLUE3D case, i.e., add a procModifier w/o any additional proliferation of dedicated workflows.
If/when at some point this PR algorithm will be part of the Phase2 reconstruction, it will automatically be monitored. Up until now, I believe that the procModifier is small and practical enough to allow users to investigate the new possibilities brought in by FastJet.
What do you think?

@jpata
Copy link
Contributor

jpata commented Nov 26, 2021

Generally we try to make an effort that new functionality would also be regularly tested in IBs, when possible and practical.
How many different configurations of modifiers are there for HGCAL reconstruction? Perhaps it could be a good time to introduce some workflows for the major ones.
Also, do you have some pre/post numbers on CPU and memory consumption with igprof?

@rovere
Copy link
Contributor Author

rovere commented Nov 26, 2021

Ciao @jpata
there are currently 2 procModifiers, one for CLUE3D and another one for FastJet. Well, perhaps I should have said one, since this PR is not merged.
I have numbers in the slides linked in the description. Definitely not conclusive and for sure nothing igprof related.
Yet a good comparison against what is currently run by default in release.
What are you practically suggesting to do in order to integrate this PR?

@jpata
Copy link
Contributor

jpata commented Nov 27, 2021

  • a test workflow
  • igprof CPU and MEM numbers

@jpata
Copy link
Contributor

jpata commented Nov 29, 2021

An alternative to a workflow is that you enable the modifier here temporarily, we run the tests and get some numbers, then you disable the modifier again.

@jpata
Copy link
Contributor

jpata commented Dec 6, 2021

kind ping on this.

enable the modifier here temporarily, we run the tests and get some numbers, then disable the modifier again.

@jpata
Copy link
Contributor

jpata commented Jan 6, 2022

kind ping @cms-sw/pdmv-l2

@rovere
Copy link
Contributor Author

rovere commented Jan 10, 2022

Hello there,
another kind ping @cms-sw/pdmv-l2

@bbilin
Copy link
Contributor

bbilin commented Jan 10, 2022

+1
sorry for the latency, just got back.

@rovere
Copy link
Contributor Author

rovere commented Jan 10, 2022

thanks!

@jpata
Copy link
Contributor

jpata commented Jan 10, 2022

(the sig didn't work, +1 / +pdmv should be the first line in the comment)

@bbilin
Copy link
Contributor

bbilin commented Jan 10, 2022

+pdmv

@qliphy
Copy link
Contributor

qliphy commented Jan 11, 2022

please test
just to refresh the test results

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-077fa0/21616/summary.html
COMMIT: 3a0c1b3
CMSSW: CMSSW_12_3_X_2022-01-10-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36172/21616/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: 43
  • DQMHistoTests: Total histograms compared: 3461659
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3461626
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+operations

  • Two new special TICL pattern recognition workflows defined for clue3D (not related to this PR, but welcome anyhow) and FastJet
  • They have been succesfully tested offline, and should become available to bot once this PR will be merged

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

static void fillPSetDescription(edm::ParameterSetDescription& iDesc);

private:
edm::ESGetToken<CaloGeometry, CaloGeometryRecord> caloGeomToken_;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be const (but it is not fundamental, and I won't reset so many signatures only for that...)

@perrotta
Copy link
Contributor

+1

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.

8 participants