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 TICL Association Map and update TICL event content #46700

Merged

Conversation

felicepantaleo
Copy link
Contributor

@felicepantaleo felicepantaleo commented Nov 14, 2024

This pull request introduces a comprehensive enhancement of the TICLAssociationMap class and updates the modules that depend on it to improve flexibility, readability, and performance for association handling within the TICL framework.

  • Refactoring of TICLAssociationMap:
    • Introduced new wrapper types FractionType and SharedEnergyType to differentiate between fraction and shared energy values more clearly.
    • Created a generic AssociationElement class to manage indices and associated values, including methods for fractional and shared energy access and accumulation operations.
    • Restructured map definitions to make use of AssociationElement for better encapsulation and flexibility in handling different types of associations.
    • Enhanced sort() Method in AssociationMap:
      • Tie-Breaker for Reproducibility: The sort() method now includes a tie-breaker based on indices when sorting elements with equal values (e.g., score, fraction, or sharedEnergy). This ensures that the sorting is reproducible and consistent across runs.
      • Custom Comparator Support: An overloaded sort() method has been added to accept a custom comparison function. This provides users with the flexibility to define their own sorting criteria based on specific needs.
    • Initialization of AssociationElement with Invalid State:
    • Invalid Default State: Modified the AssociationElement class to initialize elements in an invalid state by default. This is achieved by setting the index_ to std::numeric_limits<unsigned int>::max() and value_ to -1.f for relevant types.
    • Updated various producers and consumers of the TICLAssociationMap to work with the enhanced API.
    • Replaced direct vector manipulation with the new AssociationElement for consistency and code clarity.

Bug fixes:

  • The maps are now sorted by score in ascending order, and the sorting is made reproducible by including the tie-breaker based on indices. This fixes the wrong behavior shown in DQM plots of best associated trackster and second best associated trackster
  • Fixed Access Issues: In HGVHistoProducerAlgo.cc, replaced direct indexing with the .at() method when accessing the scToCpMap.
  • Fixed insert() in one-to-one maps
  • Validation and Testing:
    • Adjusted validation and monitoring code in modules such as HGCalValidator and TICLCandidateValidator to align with the new data structures and API changes.
  • Make the association maps persistent.
  • Include SimTauProducer in the event content for ticlv5

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 14, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46700/42646

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoHGCal/Configuration (reconstruction, upgrade)
  • RecoHGCal/TICL (reconstruction, upgrade)
  • SimCalorimetry/HGCalAssociatorProducers (upgrade, simulation)
  • SimDataFormats/Associations (simulation)
  • Validation/HGCalValidation (dqm)

@Moanwar, @antoniovagnerini, @civanch, @cmsbuild, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @rseidita, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@apsallid, @bsunanda, @cseez, @fabiocos, @forthommel, @hatakeyamak, @lecriste, @lgray, @martinamalberti, @missirol, @mmusich, @pfs, @rovere, @sameasy, @sethzenz, @sobhatta, @vandreev11, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@felicepantaleo
Copy link
Contributor Author

test parameters:

  • workflow_opts= -w upgrade
  • workflow = 29896.203, 29691.203, 29691.204
  • enable = hlt_p2_integration, hlt_p2_timing

@felicepantaleo
Copy link
Contributor Author

@cmsbuild please test

@felicepantaleo
Copy link
Contributor Author

type hgcal

@cmsbuild cmsbuild added the hgcal label Nov 14, 2024
@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 88KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fd9ac4/42852/summary.html
COMMIT: a0a2faa
CMSSW: CMSSW_14_2_X_2024-11-13-2300/el8_amd64_gcc12
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46700/42852/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

  • HLT P2 Timing: chart

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3642837
  • DQMHistoTests: Total failures: 1129
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3641688
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 216 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@rovere
Copy link
Contributor

rovere commented Nov 15, 2024

Did you expect regression on the score? As far as I can see, this is mostly (only?) evident in the sim2reco since the regression affects purity and efficiency but not fake and merge.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46700/42862

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2024

Pull request #46700 was updated. @Moanwar, @antoniovagnerini, @civanch, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @rseidita, @srimanob, @subirsarkar can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2024

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fd9ac4/43193/summary.html
COMMIT: c5bea1b
CMSSW: CMSSW_15_0_X_2024-12-02-1100/el8_amd64_gcc12
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46700/43193/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

  • HLT P2 Timing: chart

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3801299
  • DQMHistoTests: Total failures: 5343
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3795936
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 216 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: found differences in 1 / 47 workflows

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 3, 2024

+1

@civanch
Copy link
Contributor

civanch commented Dec 3, 2024

+1

@Moanwar
Copy link
Contributor

Moanwar commented Dec 3, 2024

+Upgrade

@antoniovagnerini
Copy link

+dqm

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2024

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

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1ad3369 into cms-sw:master Dec 7, 2024
14 checks passed
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