Adding an HLT filter based on PPS local track multiplicity conditions for selection of events for PPS PCL #34827
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34827/24606
|
|
A new Pull Request was created by @maaraujo94 for master. It involves the following packages:
@Martin-Grunewald, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
|
||
| // strip map definition | ||
| // diamond map definition | ||
|
|
There was a problem hiding this comment.
Why are there no strip/diam settings?
There was a problem hiding this comment.
We have the strip and diam options in place but not fully defined. This is done in case there is a future decision to include these detectors in the filter as well. As they are unused for the current implementation of the filter, all references to strip / diam conditions can be removed for clarity.
| } | ||
|
|
||
| // Then filter on strips (2016-17) selection | ||
| if (!strip_filter_.empty()) { |
There was a problem hiding this comment.
strip is always empty as it is not being set beforehand
| if (strip_filter_.count(rpv.id) == 0) | ||
| continue; | ||
| auto& fltr = strip_filter_.at(rpv.id); | ||
| fltr = true; |
There was a problem hiding this comment.
If fltr is set to true always, then why the previous line?
There was a problem hiding this comment.
I am not sure what this comment is pointing out, but as mentioned in the previous reply, the strip / diam conditions can be removed, which would remove the problem for these detectors. Does this issue also apply to the case of the pixels? If so, please elaborate on what the issue is as I don't understand it at the moment.
There was a problem hiding this comment.
You set fltr to strip_filter something, and then the line immediatly afterwards you set fltr to true, so it gets reassigned a constant value...
| } | ||
|
|
||
| // Finally filter on diamond (2016+) selection | ||
| if (!diam_filter_.empty()) { |
There was a problem hiding this comment.
diam is always empty as it is not being set beforehand
| if (diam_filter_.count(rpv.id) == 0) | ||
| continue; | ||
| auto& fltr = diam_filter_.at(rpv.id); | ||
| fltr = true; |
There was a problem hiding this comment.
If fltr is set to true always, then why the previous line?
| // maps for assigning filter pass / fail | ||
| std::unordered_map<uint32_t, bool> pixel_filter_; | ||
| std::unordered_map<uint32_t, bool> strip_filter_; | ||
| std::unordered_map<uint32_t, bool> diam_filter_; |
There was a problem hiding this comment.
It seems these 3 maps are just used as bools to switch on/off IDs, so these variables could become (const) class members and initialised as such in the class, rather than being local variables.
There was a problem hiding this comment.
The maps are used as bools to store whether the track multiplicity of each detector is within the max and min number of tracks. The final filter condition combines the result for each detector in a single logical expression, depending on which type of filtering is done.
Given their purpose, I'm not sure how to define the maps as const class members and initialize them in the class, and still be able to change their value when the filter checks track multiplicity conditions. Could you give an example of how to implement this?
|
@maaraujo94 Would you please address above comments? |
|
Thank you for the reminder. I have addressed the comments on the code and will await responses to these, then implement the necessary changes and push the updates. |
|
Please remove placeholder/unused code. |
| pixel_filter_[CTPPSPixelDetId(0, 2, 3)] = false; | ||
| pixel_filter_[CTPPSPixelDetId(1, 0, 3)] = false; | ||
| pixel_filter_[CTPPSPixelDetId(1, 2, 3)] = false; | ||
|
|
There was a problem hiding this comment.
pixel_filter never gets changed after these lines, so can be class members initialised in the class.
Same for the other two Boolean arrays.
|
I have updated and pushed the code, removing the unused code for strips and diam and also removing the auxiliary variable "fltr", which is not essential and which I believe was obscuring how the filter is processed, raising issues about the definitions of the maps of booleans. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34827/24748
|
|
Pull request #34827 was updated. @Martin-Grunewald, @cmsbuild can you please check and sign again. |
|
I still think the Boolean unordred_maps should be class members and initialised in the constructor (even if not const, syntax becomes ugly), as the maps do not seem to be changed during the event processing. |
|
I have attempted this change and found that, due to the definition of the filter as "bool filter (...) const", it is not possible to alter the boolean values of the map inside the filter if they are class members (lines 98, 104, 106 of the code give compilation errors related to this). This const declaration does not seem to be removable as it's an imposition for a global EDFilter. Please advise on which changes to apply to the filter declaration and class type to circumvent this issue. |
|
Ah ok, never mind then. |
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34827/24846
|
|
Pull request #34827 was updated. @Martin-Grunewald, @cmsbuild can you please check and sign again. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34827/24847
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f2761f/17994/summary.html Comparison SummarySummary:
|
|
+1 |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-pending
pending-signatures
labels
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR implements an HLT filter which selects events based on multiplicity conditions of the proton tracks from the PPS detector. It is to be used in the PCL of the PPS for the calibration streams.
PR validation:
Filter has been run on run 2 data and has worked as expected. More information on tests done is shown here
if this PR is a backport please specify the original PR and why you need to backport that PR:
PR is not a backport.
cc: @fabferro @jjhollar @kshcheli @jan-kaspar @vavati