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

Update HLTPixelTrackFilter to store candidates #45373

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

stahlleiton
Copy link
Contributor

@stahlleiton stahlleiton commented Jul 4, 2024

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
Copy link
Contributor

cmsbuild commented Jul 4, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2024

+code-checks

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

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2024

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the expected increase of event size?

Copy link
Contributor Author

@stahlleiton stahlleiton Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@mmusich mmusich Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

mmusich commented Jul 4, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2024

+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
Copy link
Contributor

@stahlleiton
Please make a backport to 14_0_X.

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 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 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)

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

mmusich commented Jul 5, 2024

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2024

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
Copy link
Contributor

mmusich commented Jul 5, 2024

+1

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

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

void HLTPixelTrackFilter::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
makeHLTFilterDescription(desc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read correctly

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mmusich
Copy link
Contributor

mmusich commented Jul 5, 2024

unhold

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 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 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
Copy link
Contributor Author

@stahlleiton Please make a backport to 14_0_X.

I will make the backport once this PR is merged

@stahlleiton
Copy link
Contributor Author

Is there something pending to get this PR merged?

@Martin-Grunewald
Copy link
Contributor

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

@mmusich
Copy link
Contributor

mmusich commented Jul 8, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

...

@mmusich
Copy link
Contributor

mmusich commented Jul 9, 2024

Is this a showstopper?

No, please merge.

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 70af9f3 into cms-sw:master Jul 9, 2024
11 checks passed
@stahlleiton stahlleiton deleted the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 14:39
@stahlleiton stahlleiton restored the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 16:16
@stahlleiton stahlleiton deleted the HLT_UPC_CMSSW_14_1_X branch July 9, 2024 21:50
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.

6 participants