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

Replace patterns with explicit class names in TrackingTools/PatternTools/src/classes_def.xml, and move the dictionaries of GlobalErrorBase to a proper package #45623

Merged

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Aug 1, 2024

PR description:

This PR is part of series to replace the last remaining uses class pattern elements in classes_def.xml with explicit class names. In addition, it removes (now-)unnecessary code from classes.h, adds persistent="false" to all edm::Wrapper<T> instantiations and removes it from other classes, and finally moves GlobalErrorBase (and Point3DBase) related dictionaries to DataFormats/GeometryCommonDetAlgo that seemed like the right place (it seems to me these dictionaries were defined in TrackingTools/PatternTools because of typedef std::pair<GlobalPoint, GlobalError> VertexConstraint type alias used there; I also hope the "ROOT6 problem" mentioned in the comment has been solved already).

Resolves cms-sw/framework-team#980

PR validation:

Code compiles. The pattern-to-name change kept all existing definitions in the .rootmap file, and added a few more with that use the type aliases I used in the name attribute.

- edm::Wrapper<T> instantiations must be transient in a non-DataFormat package
- Other classes should not have persistent="false" (it has no effect, and can confuse)
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

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

It involves the following packages:

  • DataFormats/GeometryCommonDetAlgo (simulation)
  • TrackingTools/PatternTools (reconstruction)

@civanch, @cmsbuild, @jfernan2, @mandrenguyen, @mdhildreth can you please review it and eventually sign? Thanks.
@CeliaFernandez, @Fedespring, @GiacomoSguazzoni, @HuguesBrun, @JanFSchulte, @VinInn, @VourMa, @abbiendi, @andrea21z, @bellan, @cericeci, @dgulhan, @felicepantaleo, @gpetruc, @jhgoh, @missirol, @mmusich, @mtosi, @rovere, @trocino 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

@makortel
Copy link
Contributor Author

makortel commented Aug 1, 2024

@cmsbuild, please test

@makortel
Copy link
Contributor Author

makortel commented Aug 1, 2024

type -changes-dataformats

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2024

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-eb82ed/40730/summary.html
COMMIT: 6544cff
CMSSW: CMSSW_14_1_X_2024-08-01-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45623/40730/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 4.784.78_WElSkim2012D/step2_WElSkim2012D.log

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 236 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3530500
  • DQMHistoTests: Total failures: 4339
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3526141
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
  • Checked 200 log files, 170 edm output root files, 46 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

makortel commented Aug 2, 2024

The failure in 4.78 is a file open error. Comparison differences are related to #39803

@civanch
Copy link
Contributor

civanch commented Aug 4, 2024

please test

there are PRs today, which pass tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2024

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-eb82ed/40748/summary.html
COMMIT: 6544cff
CMSSW: CMSSW_14_1_X_2024-08-04-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45623/40748/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: 88 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3530500
  • DQMHistoTests: Total failures: 2464
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3528016
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
  • Checked 200 log files, 170 edm output root files, 46 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Aug 5, 2024

+1

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 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 be automatically merged.

@cmsbuild cmsbuild merged commit 16e712c into cms-sw:master Aug 5, 2024
11 checks passed
@makortel makortel deleted the removePatternTrackingToolsPatternTools branch August 5, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment