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

[PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484

Merged
merged 70 commits into from
Dec 11, 2020

Conversation

forthommel
Copy link
Contributor

PR description:

This PR is a follow-up of #30252 after the merging of #31383 porting the PPS geometry into the DD4hep world. It includes a new set of DDLs for 2021 scenario (preparing for the addition of a new timing station), before integration onto the geometry DB payload.
The DetGeomDesc geometry parser is therefore modified to host this additional station, better exploiting the copy numbers in a modified hierarchy. As a result, this latter requires the addition of a "legacy" isRun2 flag to allow the previous DD/DB geometry format.

As a side development, the track reconstruction plugin is adapted to dynamically create a track fitting algorithm for each new pot/station observed in the rechits stream (i.e. timing pots are not hardcoded anymore in plugin implementation).
Algorithms being stored as an associative container (single pot detid → algorithm) of this latter, a hash function is defined for the CTPPSDetId object used as an indexing variable.

@fabferro @jan-kaspar @ghugo83

PR validation:

Same validation procedure as #30996. Observed no difference in run 2 CTPPSDiamond* objects with the definition of the new station.
Overlaps detected (using the g4OverlapCheck tool) equivalent to the ones already present in run 2 geometry (the current second station implementation is a naive copy-paste of this latter), and will be corrected in a follow-up PR. This PR being intended for an update of the reconstruction geometry payload (not the simulation one) it should not be problematic. Experts are warmly welcomed to comment.

if this PR is a backport please specify the original PR and why you need to backport that PR: N/A

Before submitting your pull requests, make sure you followed this checklist:

Vetchu and others added 30 commits July 14, 2020 15:47
…n algorithm (timing/tracking associatioN)"

This reverts commit c1ac29c.
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2020

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d0bc69/11478/summary.html
CMSSW: CMSSW_11_3_X_2020-12-09-1100/slc7_amd64_gcc900

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2020

Comparison results are now available
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d0bc69/11478/summary.html
CMSSW: CMSSW_11_3_X_2020-12-09-1100/slc7_amd64_gcc900

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 46 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2747014
  • DQMHistoTests: Total failures: 18
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2746973
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 35 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 153 log files, 37 edm output root files, 36 DQM output files

@forthommel
Copy link
Contributor Author

Seems that the comparisons are compatible with #31484 (comment) (a few more WF seems to have appeared in the meantime though?)

@jfernan2
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

+1

  • Just rebased, and applied a few changes not reco related, since last reco signature in [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484 (comment)
  • In the whole PR the only change in reco is for the CTPPSDiamondLocalTrackFitter plugin, which now dynamically creates a track fitting algorithm for each new pot/station observed in the rechits stream (instead of hardcoding every time the two "45" and "56" ones, as it was done before): this will also allow dynamically taking into account of possible new ones
  • Being they created dynamically, the size of those DiamondLocalTracks is now 0 when they are not present in the data or in the MC setup, as expected (with respect to the previous comparisons, there are a few more MC workflows, and therefore a larger number of workflows with differences are reported by the automatic comparison)
  • Jenkins tests do not show other differences

@forthommel
Copy link
Contributor Author

@forthommel
Copy link
Contributor Author

@christopheralanwest, @tlampen, @pohsun, @yuanchao, @ggovi, @mdhildreth, @civanch, as usual, let me/us know if you need any help to ease your review.

@ggovi
Copy link
Contributor

ggovi commented Dec 10, 2020

+1

@forthommel
Copy link
Contributor Author

Many thanks, @ggovi !
@christopheralanwest, @tlampen, @pohsun, @yuanchao, @mdhildreth, @civanch, kindly reminder :)

@silviodonato
Copy link
Contributor

merge
@cms-sw/simulation-l2 already signed before the last rebase
@cms-sw/alca-l2 had to review only a trivial change CalibPPS/ESProducers/plugins/CTPPSCompositeESSource.cc
(please let us know if you have any comments)

I've opened an issue for a small followup PR regarding @makortel 's comment
#32448

Even if Validation/CTPPS/python/simu_config/year_201*_cff.py are currently used for a private production, they should be safe (see #31484 (comment))

@cmsbuild cmsbuild merged commit 7170574 into cms-sw:master Dec 11, 2020
@forthommel forthommel deleted the pps-timing_2021_geom-11_2_X branch December 11, 2020 09:39
@forthommel
Copy link
Contributor Author

Thanks a lot, @silviodonato ! Off for a few more PRs now!

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