search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.29k forks source link

Remaining issues after the merging of "L1T Phase-2, remove old simulation and adapt DQM (#38442)" #39194

Closed perrotta closed 2 years ago

perrotta commented 2 years ago

The PR in object was merged in time for 12_5_0_pre5, but there were several issues that remained open, and that should all get fixed as soon as possible, and in any case before the final 12_5_0. If I did not forget anything, the list of the still open issues is the following:

@cecilecaillol @trtomei @BenjaminRS ... @fwyzard @jpata @missirol @Martin-Grunewald @AdrianoDee @srimanob ...

perrotta commented 2 years ago

assign @cms-sw/l1-l2

cmsbuild commented 2 years ago

A new Issue was created by @perrotta Andrea Perrotta.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

missirol commented 2 years ago

assign l1

(based on https://github.com/cms-sw/cmssw/issues/39194#issuecomment-1227414925 ; thanks @perrotta for opening the issue.)

Regarding item-1, I think the main point is the introduction of a naming convention, e.g. l1t[Module] (in addition to starting with lowercase letters), as mentioned in https://github.com/cms-sw/cmssw/pull/38442#discussion_r952263162 https://github.com/cms-sw/cmssw/pull/38442#discussion_r952277982 https://github.com/cms-sw/cmssw/pull/38442#issuecomment-1223694816

cmsbuild commented 2 years ago

New categories assigned: l1

@epalencia,@rekovic,@cecilecaillol you have been requested to review this Pull request/Issue and eventually sign? Thanks

missirol commented 2 years ago

assign hlt

Items-2,3,4 concern the HLT Phase-2 menu (@trtomei).

cmsbuild commented 2 years ago

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

cecilecaillol commented 2 years ago

@folguera Could you please have a look at item 6 (Phase-2 muon DQM)?

cecilecaillol commented 2 years ago

Item 5: The differences in cutset are only formatting, there is no change to the values

perrotta commented 2 years ago

Item 5: The differences in cutset are only formatting, there is no change to the values

Thank you Cecile. There were a few differences in the parameters included in the cutset PSet of the module L1TrackSelectionProducerExtended between the original commits of yours in the PR and what finally got merged. The modifications that got removed in your latest commits are:

  cutset = dict(ptmin = 3.,# pt must be greater than this value, [GeV]
                reducedBendChi2Max = 2.4, # bend chi2 must be less than this value
                reducedChi2RZMax = 10.0, # chi2rz/dof must be less than this value
                reducedChi2RPhiMax = 40.0, # chi2rphi/dof must be less than this value
                deltaZMax = [3.0, 3.0, 3.0, 3.0, 3.0, 3.0], # delta z must be less than these values, there will be one less value here than in deltaZMaxEtaBounds, [cm]
            ),

That item 5 intended just to check with you whether the removal of those modifications of the parametere were intentional (then everything is fine), or if they were just forgotten at the end (and therefore they should be reimplemented if so).

cecilecaillol commented 2 years ago

Item 5: The differences in cutset are only formatting, there is no change to the values

Thank you Cecile. There were a few differences in the parameters included in the cutset PSet of the module L1TrackSelectionProducerExtended between the original commits of yours in the PR and what finally got merged. The modifications that got removed in your latest commits are:

  cutset = dict(ptmin = 3.,# pt must be greater than this value, [GeV]
                reducedBendChi2Max = 2.4, # bend chi2 must be less than this value
                reducedChi2RZMax = 10.0, # chi2rz/dof must be less than this value
                reducedChi2RPhiMax = 40.0, # chi2rphi/dof must be less than this value
                deltaZMax = [3.0, 3.0, 3.0, 3.0, 3.0, 3.0], # delta z must be less than these values, there will be one less value here than in deltaZMaxEtaBounds, [cm]
            ),

That item 5 intended just to check with you whether the removal of those modifications of the parametere were intentional (then everything is fine), or if they were just forgotten at the end (and therefore they should be reimplemented if so).

Sorry, I am still confused. I see these parameters are present and unchanged in the current L1TrackSelectionProducerExtended configuration: https://github.com/cecilecaillol/cmssw/blob/f6c5d02e18f03e70fc6f95a58c8e0782770d356e/L1Trigger/L1TTrackMatch/python/L1TrackSelectionProducer_cfi.py#L33-L44. Am I missing something?

perrotta commented 2 years ago

Sorry, I am still confused. I see these parameters are present and unchanged in the current L1TrackSelectionProducerExtended configuration: https://github.com/cecilecaillol/cmssw/blob/f6c5d02e18f03e70fc6f95a58c8e0782770d356e/L1Trigger/L1TTrackMatch/python/L1TrackSelectionProducer_cfi.py#L33-L44. Am I missing something?

I was probably looking at some intermediate version of that file, where the cutset PSet was missing in the cloned config... Nevertheless, the original comment was to remove the type specifications in the parameters (whch is safer, cleaner, etc.). Therefore, the cloned config can be rewritten as

L1TrackSelectionProducerExtended = L1TrackSelectionProducer.clone(
  l1TracksInputTag = ("L1GTTInputProducerExtended","Level1TTTracksExtendedConverted"),
  outputCollectionName = "Level1TTTracksExtendedSelected",
  cutset = dict(ptmin = 3.,# pt must be greater than this value, [GeV]
                reducedBendChi2Max = 2.4, # bend chi2 must be less than this value
                reducedChi2RZMax = 10.0, # chi2rz/dof must be less than this value
                reducedChi2RPhiMax = 40.0, # chi2rphi/dof must be less than this value
                deltaZMax = [3.0, 3.0, 3.0, 3.0, 3.0, 3.0], # delta z must be less than these values, there will be one less value here than in deltaZMaxEtaBounds, [cm]
            ),
  useDisplacedTracksDeltaZOverride = 3.0 # Use promt/displaced tracks
)
cecilecaillol commented 2 years ago

@trtomei I took care of item 2. Can you please address items 3 and 4?

trtomei commented 2 years ago

Yes, it's on my TODO list for today :)

perrotta commented 2 years ago

urgent (To make it visible in the list of issues: item 6, in particular, keeps messing the Phase2 DQM comparisons in the PR tests)

trtomei commented 2 years ago

Pull requests to address items 3 and 4 are here: https://github.com/cms-sw/cmssw/pull/39280 https://github.com/cms-sw/cmssw/pull/39281

perrotta commented 2 years ago

Items (2) and (3) addressed by https://github.com/cms-sw/cmssw/pull/39280 and ticked in the main description

trtomei commented 2 years ago

@perrotta #39280 actually addresses items (3) and (4), item (2) was previously fixed by Cecile.

perrotta commented 2 years ago

39283 was not able to fix item 6, because also after its merge in CMSSW_12_6_X_2022-09-02-1100 the PR tests continue to show non reproducible results for L1T muon global efficiencies, both gor SA and Tracker muons, e.g.:

image

This must be investigated further by @cms-sw/dqm-l2

perrotta commented 2 years ago

Itemo 6 got fixed by #39301: thank you @missirol for it!

cecilecaillol commented 2 years ago

Items 1, 2, and 5 fixed in https://github.com/cms-sw/cmssw/pull/38442

cecilecaillol commented 2 years ago

+l1

perrotta commented 2 years ago

A fix for item 5 was included in #39244 All them have been addressed then, and this issue can be closed

missirol commented 2 years ago

+hlt

cmsbuild commented 2 years ago

This issue is fully signed and ready to be closed.