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

Improve MC truth information in TICL, Sim-Reco Associators, TICLDumper #42518

Merged
merged 12 commits into from
Oct 27, 2023

Conversation

waredjeb
Copy link
Contributor

@waredjeb waredjeb commented Aug 9, 2023

This PR is meant to improve the Monte Carlo truth information within the TICL Framework.

  • Add RecoTrack index to collection of SimTracksters. This is done by using the SimTrack-to-TrackingParticle association map and the TrackingParticle-to-RecoTrack association. The SimTrack is known from the CaloParticle/SimCluster that seeds the creation of the SimTrackster

  • Add PU SimTrackster. This is a single SimTrackster built from all the remaining contributions from the LayerClusters not used in the SimTracksters. This is only used to estimate the pileup contamination in the recoObjects

  • Add SimTICLCandidate: This is a new object created within the SimTracksterProducer. This object is meant to be the best Particle Flow reconstruction we can achieve with the available objects, and is going to be used for validation of the full TICL chain. This object is created for each CaloParticle and contains the list of SimTracksters beloging to the same CaloParticle and the best recoTrack associated to the CaloParticle (plus each SimTrackster knows its RecoTrack) [1]

  • Associators: Adding associators[2] to EventContent.

  • TICLDumper: EDAnalyzer for saving TICL information in TTrees and be used out of CMSSW. Extensively used by TICL group for several studies and developments. Can be enabled by using customiseTICLFromReco in step3

  • Trackster DataFormat optimization

  • [1] https://indico.cern.ch/event/1234092/sessions/473632/attachments/2582424/4470565/TICLMeeting_09_02_23.pdf

  • [2] https://hgcal.web.cern.ch/Validation/TracksterValidation/

Validation

  • runTheMatrix.py -l limited -i all --ibeos
  • private testing to inspect TICLDumper content

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2023

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42518/36526

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42518/36530

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2023

A new Pull Request was created by @waredjeb (Wahid Redjeb) for master.

It involves the following packages:

  • DataFormats/HGCalReco (upgrade, reconstruction)
  • RecoHGCal/Configuration (upgrade, reconstruction)
  • RecoHGCal/TICL (upgrade, reconstruction)
  • SimCalorimetry/HGCalAssociatorProducers (upgrade, simulation)
  • SimDataFormats/Associations (simulation)
  • SimDataFormats/Track (simulation)
  • SimGeneral/TrackingAnalysis (simulation)
  • SimTracker/Configuration (simulation)
  • Validation/Configuration (dqm, simulation)

@civanch, @pmandrik, @nothingface0, @emanueleusai, @mdhildreth, @mandrenguyen, @cmsbuild, @AdrianoDee, @srimanob, @clacaputo, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@echabert, @VourMa, @felicepantaleo, @forthommel, @abbiendi, @robervalwalsh, @CeliaFernandez, @bsunanda, @threus, @mmusich, @slomeo, @youyingli, @missirol, @andrea21z, @JanFSchulte, @jhgoh, @dgulhan, @apsallid, @prolay, @HuguesBrun, @cericeci, @trocino, @GiacomoSguazzoni, @rovere, @VinInn, @hatakeyamak, @ebrondol, @mtosi, @fabiocos, @gbenelli, @Fedespring, @sobhatta, @lecriste, @sameasy this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@emanueleusai
Copy link
Member

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f27b14/34220/summary.html
COMMIT: d20c3e1
CMSSW: CMSSW_13_3_X_2023-08-10-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42518/34220/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@smuzaffar
Copy link
Contributor

The failing test was for el8_amd64_gcc11 i.e cms/42518/el8_amd64_gcc11/relvals/input. I guess the tests were run for gcc11 (when gcc11 was still the production compiler) and then we move to gcc12. So the gcc11 tests were never re-run and kept the old state. Anyway, I have force set the tests to pass. This PR is good to go in

@felicepantaleo
Copy link
Contributor

thanks @smuzaffar

@felicepantaleo
Copy link
Contributor

a kind ping @emanueleusai @mandrenguyen

@emanueleusai
Copy link
Member

hi, as of oct 1st I don't do PR reviews anymore, please refer to the current dqmdc conveners for the dqm signature of this pr

@felicepantaleo
Copy link
Contributor

@cms-sw/dqm-l2 ?

@felicepantaleo
Copy link
Contributor

@cms-sw/reconstruction-l2 in case also the reconstruction reviewer has changed.

@mandrenguyen
Copy link
Contributor

There is still a pending change requested by Sal.

@waredjeb
Copy link
Contributor Author

There is still a pending change requested by Sal.

Hi @mandrenguyen I believe I have already added the requested change (https://github.com/cms-sw/cmssw/pull/42518/files#diff-8c640197bff70f21e9374b2524ed4b091821e12ead6340f71bb51ca5719e4974R90). Please, let me know if anything else is needed.

@waredjeb waredjeb requested a review from rappoccio October 23, 2023 09:52
@felicepantaleo
Copy link
Contributor

I have asked @rappoccio to lift the changes requested as they have been addressed.
We have addressed all the comments within the day they were asked.
This PR needs to be merged as without it the development and sample production for the HGCAL reconstruction in the release is very difficult for the quantity of packages needed to merge it.
It is also blocking other developments that depend on it.

@rappoccio
Copy link
Contributor

@mandrenguyen I resolved the conversation, thanks!

@mandrenguyen
Copy link
Contributor

+1

@rappoccio
Copy link
Contributor

@cms-sw/dqm-l2 Can you please review and sign, this is blocking integration.

@tjavaid
Copy link

tjavaid commented Oct 26, 2023

Hi @waredjeb , looking at DQM bin by bin comparisons, I see, in a workflow there are a certain number of distributions that show some change. The new distributions closely match in most of the cases but there few examples like:
image

Is it expected?

@mmusich
Copy link
Contributor

mmusich commented Oct 26, 2023

@tjavaid, wf 12434.7 is unfortunately known to be unstable, see also #39803 . You can ignore the differences for the purpose of this PR.

@tjavaid
Copy link

tjavaid commented Oct 26, 2023

+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 now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@felicepantaleo
Copy link
Contributor

@rappoccio @antoniovilela , it would be great if this could be merged before the next prerelease is built.

@antoniovilela
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.