Update HLTPixelTrackFilter to store candidates #45373

stahlleiton · 2024-07-04T12:38:56Z

PR description:

This PR updates the HLTPixelTrackFilter to store the online candidates used in the filter. One problem faced in the HIN UPC low pT pixel track triggers used in the PbPb 2023 data tacking, is that the candidates were not stored in the event since the path was relying on a standard filter (CandViewCountFilter) instead of a filter derived from HLTFilter class, which has made the study of the track trigger efficiency challenging. To avoid this in the 2024 data tacking, the HLTPixelTrackFilter is updated to derive from the HLTFilter class and have the option to store the candidates, so it could be used in the HIN UPC triggers.

@vince502 @mandrenguyen

PR validation:

Tested locally by re-emulating the HLT menu and replacing the hltSinglePixelTrackLowPtForUPC module in the HLT_HIon_cff.py menu using the HLTPixelTrackFilter.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild · 2024-07-04T12:39:18Z

cms-bot internal usage

cmsbuild · 2024-07-04T12:44:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45373/40816

This PR adds an extra 16KB to repository

cmsbuild · 2024-07-04T12:44:41Z

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

It involves the following packages:

HLTrigger/special (hlt)

@Martin-Grunewald, @cmsbuild, @mmusich can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @mmusich, @silviodonato this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2024-07-04T12:49:59Z

HLTrigger/special/plugins/HLTPixelTrackFilter.cc


  unsigned int numTracks = trackColl->size();
+  for (size_t i = 0; i < numTracks; i++)
+    filterproduct.addObject(trigger::TriggerTrack, reco::RecoChargedCandidateRef(trackColl, i));


what's the expected increase of event size?

This is meant to be run in PbPb ultra-peripheral collisions, in the low pT pixel track triggers that require a maximum of 400 pixel clusters (https://github.com/cms-sw/cmssw/blob/CMSSW_13_2_X/HLTrigger/Configuration/python/HLT_HIon_cff.py#L25389) where there are most of the time <10 tracks per event (mostly the events are either empty or have around 1 or 2 tracks from UPC coherent photo-production processes). I will try to derive the increase in event size by re-emulating on a 2023 PbPb HIForward dataset.

This is meant to be run in PbPb ultra-peripheral collisions

Since someone in the future may decide to use this filter on a completely different set of events, would it make sense to add a flag to disable this feature ? (e.g. a boolean fillTriggerObjectWithRefs)

I agree that this is opening a dangerous door. Please add a parameter to enable that feature (and leave it disabled by default).

Sorry, no additional parameter; this is what saveTags = cms.bool( True ), (or False) is for (parameter defined in the base class HLTFilter).

it will likely be forgotten to update/propagate it in this plugin where you dropped the call to

Why is that an issue? It will then throw at runtime because of missing parameters, and immediately spotted.

So let's avoid that by keeping this call as it is kept for all others.

Mmmh, no. That generated the risk of having users to accidentally use this filter on heavy events and consequently accidentally increase substantially the event size, unless it is recalled when the corresponding configuration is integrated.

Sorry, I just wholly disagree! In any case, if that is really an issue, and for me it is not, we should definitely revisit ALL saveTags seetings of ALL instances of all HLTFilters in the menu, and check with proponents if TRUE is really needed or can be flipped to false. There is no special case in this plugin.

In any case, if that is really an issue, and for me it is not, we should definitely revisit ALL saveTags seetings of ALL instances of all HLTFilters in the menu.

I am not going to disagree to that.

There is no special case in this plugin.

I think no other physics object in the menu can attain the multiplicity of pixel tracks, but maybe I am overestimating the impact on event size.

mmusich · 2024-07-04T15:14:21Z

@cmsbuild, please test

cmsbuild · 2024-07-04T18:35:46Z

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ee4540/40228/summary.html
COMMIT: a07f4db
CMSSW: CMSSW_14_1_X_2024-07-04-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45373/40228/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.

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3345088
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3345065
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 202 log files, 165 edm output root files, 48 DQM output files
TriggerResults: no differences found

Martin-Grunewald · 2024-07-05T07:00:23Z

@stahlleiton
Please make a backport to 14_0_X.

Martin-Grunewald · 2024-07-05T07:00:35Z

+1

cmsbuild · 2024-07-05T07:00:46Z

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 (and backports should be raised in the release meeting by the corresponding L2)

missirol

A couple of questions.

missirol · 2024-07-05T07:14:18Z

HLTrigger/special/plugins/HLTPixelTrackFilter.cc

+                                    const edm::EventSetup& iSetup,
+                                    trigger::TriggerFilterObjectWithRefs& filterproduct) const {
+  // All HLT filters must create and fill an HLT filter object,
+  // recording any reconstructed physics objects satisfying (or not)


I think often times the "trigger objects" are added to the event only if the filter accepts the event. In this case they are added irrespective of the decision of the filter. Is that intended ?

This is true often, but not always. It is up to the use case of the specific filter.

missirol · 2024-07-05T07:19:09Z

HLTrigger/special/plugins/HLTPixelTrackFilter.cc


  unsigned int numTracks = trackColl->size();
+  for (size_t i = 0; i < numTracks; i++)
+    filterproduct.addObject(trigger::TriggerTrack, reco::RecoChargedCandidateRef(trackColl, i));


This is meant to be run in PbPb ultra-peripheral collisions

Since someone in the future may decide to use this filter on a completely different set of events, would it make sense to add a flag to disable this feature ? (e.g. a boolean fillTriggerObjectWithRefs)

mmusich · 2024-07-05T07:21:44Z

hold

cmsbuild · 2024-07-05T07:22:03Z

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

mmusich · 2024-07-05T07:23:14Z

+1

question at #45373 (comment) was not yet answered.

mmusich · 2024-07-05T07:39:37Z

HLTrigger/special/plugins/HLTPixelTrackFilter.cc

@@ -50,6 +52,7 @@ HLTPixelTrackFilter::~HLTPixelTrackFilter() = default;

 void HLTPixelTrackFilter::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
  edm::ParameterSetDescription desc;
+  makeHLTFilterDescription(desc);


If I read correctly

cmssw/HLTrigger/HLTcore/src/HLTFilter.cc

Line 25 in 94ac0f5

void HLTFilter::makeHLTFilterDescription(edm::ParameterSetDescription& desc) { desc.add<bool>("saveTags", true); }

this will enable saving the tracks by default (which is what in my opinion is dangerous)

Some historical background: many(!) years ago TSG moved the default from FALSE to TRUE (too many trigger proponents forgetting to flip to true - and complaining - , while filesize increase was miniscule, as only 4-vector info of objects is packed up).

OK, I see - thanks. Still I think it would be good to have a quantitative measurement.

mmusich · 2024-07-05T12:49:40Z

unhold

cmsbuild · 2024-07-05T12:50:03Z

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 (and backports should be raised in the release meeting by the corresponding L2)

stahlleiton · 2024-07-05T14:14:30Z

@stahlleiton Please make a backport to 14_0_X.

I will make the backport once this PR is merged

stahlleiton · 2024-07-08T10:44:25Z

Is there something pending to get this PR merged?

Martin-Grunewald · 2024-07-08T11:02:38Z

Waiting on @cms-sw/orp-l2 ...

mmusich · 2024-07-08T14:06:30Z

Is there something pending to get this PR merged?

I'd still like to understand why #45373 (comment) (and then setting the concrete instance of the filter to True in the HIon menu) would be an issue.

antoniovilela · 2024-07-08T22:27:22Z

Is there something pending to get this PR merged?

I'd still like to understand why #45373 (comment) (and then setting the concrete instance of the filter to True in the HIon menu) would be an issue.

Is this a showstopper?

antoniovilela · 2024-07-08T22:46:05Z

Waiting on @cms-sw/orp-l2 ...

...

mmusich · 2024-07-09T06:20:16Z

Is this a showstopper?

No, please merge.

antoniovilela · 2024-07-09T14:19:59Z

+1

Update HLTPixelTrackFilter for 2024 PbPb UPC

a07f4db

cmsbuild added this to the CMSSW_14_1_X milestone Jul 4, 2024

cmsbuild added hlt-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jul 4, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 4, 2024

mmusich reviewed Jul 4, 2024

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Jul 4, 2024

cmsbuild added tests-approved and removed tests-started labels Jul 4, 2024

cmsbuild added hlt-approved fully-signed and removed hlt-pending pending-signatures labels Jul 5, 2024

missirol reviewed Jul 5, 2024

View reviewed changes

cmsbuild removed the fully-signed label Jul 5, 2024

cmsbuild added pending-signatures hold labels Jul 5, 2024

mmusich reviewed Jul 5, 2024

View reviewed changes

cmsbuild removed pending-signatures hold labels Jul 5, 2024

cmsbuild added the fully-signed label Jul 5, 2024

cmsbuild added orp-approved and removed orp-pending labels Jul 9, 2024

cmsbuild merged commit 70af9f3 into cms-sw:master Jul 9, 2024
11 checks passed

stahlleiton deleted the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 14:39

stahlleiton restored the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 16:16

cmsbuild mentioned this pull request Jul 9, 2024

Set c++20 as a default standard cms-sw/cmsdist#9288

Merged

stahlleiton deleted the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 21:50

stahlleiton mentioned this pull request Jul 9, 2024

[14_0_X] Update HLTPixelTrackFilter to store candidates #45417

Merged

This was referenced Jul 11, 2024

[GCC13] Update gcc to 13.3.0 cms-sw/cmsdist#9270

Open

[GCC13] Patch applied for c++20 cms-sw/cmsdist#9295

Merged

Update HLTPixelTrackFilter to store candidates #45373

Update HLTPixelTrackFilter to store candidates #45373

Conversation

stahlleiton commented Jul 4, 2024 • edited Loading

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Jul 4, 2024 • edited Loading

cmsbuild commented Jul 4, 2024

cmsbuild commented Jul 4, 2024

Choose a reason for hiding this comment

stahlleiton Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Grunewald Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich commented Jul 4, 2024

cmsbuild commented Jul 4, 2024

Comparison Summary

Martin-Grunewald commented Jul 5, 2024

Martin-Grunewald commented Jul 5, 2024

cmsbuild commented Jul 5, 2024

missirol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich commented Jul 5, 2024

cmsbuild commented Jul 5, 2024

mmusich commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich commented Jul 5, 2024

cmsbuild commented Jul 5, 2024

stahlleiton commented Jul 5, 2024

stahlleiton commented Jul 8, 2024

Martin-Grunewald commented Jul 8, 2024

mmusich commented Jul 8, 2024 • edited Loading

antoniovilela commented Jul 8, 2024

antoniovilela commented Jul 8, 2024

mmusich commented Jul 9, 2024

antoniovilela commented Jul 9, 2024

stahlleiton commented Jul 4, 2024 •

edited

Loading

cmsbuild commented Jul 4, 2024 •

edited

Loading

stahlleiton Jul 4, 2024 •

edited

Loading

mmusich Jul 5, 2024 •

edited

Loading

Martin-Grunewald Jul 8, 2024 •

edited

Loading

mmusich commented Jul 8, 2024 •

edited

Loading