[HGCAL] Fix search box eta phi #39598
Conversation
The tile data structures are used everywhere in TICL and should handle both positive and negative eta values. While the one used to create layer clusters natively supports that use case, the one used in the pattern recognition algorithms does not and internally translates negative eta values into positive ones. Therefore the logic of the method searchBoxEtaPhi had to be fixed in order to handle all possible input values for etaMin, etaMax, phiMin and phiMax. The data structure for the layer clusters only needed a protection on the correctness of the eta range, while the one used within TICL required a little more work and plumbing to make it work the way it is supposed to work. To be noted that for this latter case, queries that use intervals in eta spanning both sides of HGCAL are silently ignored and will return an empty interval. The TICLLayerTile data structure can only handle one HGCAL endcap at a time.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39598/32393
|
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages:
@cmsbuild, @AdrianoDee, @srimanob, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0979e3/27981/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
type hgcal |
|
|
|
+1 |
|
+upgrade
|
|
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
| @@ -90,6 +90,9 @@ class HGCalLayerTilesT { | |||
| } | |||
|
|
|||
| std::array<int, 4> searchBoxEtaPhi(float etaMin, float etaMax, float phiMin, float phiMax) const { | |||
| if (etaMax - etaMin < 0) { | |||
There was a problem hiding this comment.
(just to check): no swap is needed here?
There was a problem hiding this comment.
Ciao @perrotta , thanks for asking. In this specific case, the swap is not needed because this data structure internally supports both positive and negative eta values. The check that the extension of the interval in eta makes sense is the only one needed. The rest is taken care of internally by the bin assignment.
|
+1 |
PR description:
Properly handle negative eta values in calls to
searchBoxEtaPhiof theTiledata structures.Rewrite unit tests to check its validity.
PR validation:
Unit tests run fine.
Expect no regression.