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

[perrotta]

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:

• Change modules and class member names to lowercase, see [L1T] Phase-2, remove old simulation and adapt DQM #38442 (comment) (see also Remaining issues after the merging of "L1T Phase-2, remove old simulation and adapt DQM (#38442)" #39194 (comment))

• Fix hltTriggerSummaryAOD, see [L1T] Phase-2, remove old simulation and adapt DQM #38442 (comment)

• change the interface of the TrackerMuon module (i.e. the configuration parameters) to take a single list of qualities, instead of the more complex approach there is now, see [L1T] Phase-2, remove old simulation and adapt DQM #38442 (review)

• Check the defaults in detQualDefault.addParameter<std::vector<int>>("qualities", {-99, -99, -99}), see [L1T] Phase-2, remove old simulation and adapt DQM #38442 (comment)

• Verify the PSet cutset in L1TrackSelectionProducerExtended , cloned from L1TrackSelectionProducer in L1Trigger/L1TTrackMatch/python/L1TrackSelectionProducer_cfi.py (as it was different in [L1T] Phase-2, remove old simulation and adapt DQM #38442 before the last commits: not clear if it was modified on purpose or by mistake)

• After the merging of [L1T] Phase-2, remove old simulation and adapt DQM #38442 we are noticing spurios differences in the efficiencies for Phase2 muons in DQM in the PR tests, see e.g. Run3-sim126 Try to adress complaints from static analyser for SimG4CMS/Calo #39209 and ECAL - Remove ExternalWork from EcalUncalibRecHitProducerGPU #39193

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

[perrotta]

assign @cms-sw/l1-l2

[cmsbuild]

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]

assign l1

(based on #39194 (comment) ; 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
#38442 (comment)
#38442 (comment)
#38442 (comment)

[cmsbuild]

New categories assigned: l1

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

[missirol]

assign hlt

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

[cmsbuild]

New categories assigned: hlt

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

[cecilecaillol]

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

[cecilecaillol]

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

[perrotta]

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]

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]

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]

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

[trtomei]

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

[perrotta]

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]

Pull requests to address items 3 and 4 are here:
#39280
#39281

[perrotta]

Items (2) and (3) addressed by #39280 and ticked in the main description

[trtomei]

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

[perrotta]

#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.:

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

[perrotta]

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

[cecilecaillol]

Items 1, 2, and 5 fixed in #38442

[cecilecaillol]

+l1

[perrotta]

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

[missirol]

+hlt

[cmsbuild]

This issue is fully signed and ready to be closed.