bug fix in vertex selection of L2TauTagNNProducer #37291
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37291/28923
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37291/28925
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
@valeriadamante , same issue with |
| std::vector<double> maxChi2_; | ||
| std::vector<double> pTSquaredSum(nv); | ||
|
|
||
| for (int j = nv - 1; j >= 0; --j) { |
There was a problem hiding this comment.
this double loop is not required.
enough to loop on the tracks, apply selection, and fill pTSquaredSum directly.
No need to sort either as the algorithms is just using the max (not even the location, just the max value of pt2sum).
|
@missirol Valeria |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37291/28945
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
@valeriadamante , as you can see from the GitHub page, the label "code-checks-rejected" is still there. After running |
|
type bugfix @valeriadamante , please fix this PR [*], so that the tests can be started. Since this is a bugfix, and HLT development is still mostly in [*] cd ${CMSSW_BASE}/src
git checkout BugFixVtx
scram b code-checks
scram b code-format
git add -u :/
git commit -m 'apply code checks'
git push my-cmssw BugFixVtx |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37291/28994
|
|
Pull request #37291 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55b7e0/23399/summary.html Comparison SummarySummary:
|
|
+hlt
@valeriadamante , please open a backport PR to |
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) |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55b7e0/23432/summary.html Comparison SummarySummary:
|
|
+1 |
PR description:
Doing tests on GPU, some issues in L2NNTag produced outputs have been reported: running twice the L2NNTag producer, it gave different results on the same events/taus. The problem has been addressed to the filling of the CNN inputs :
PatatrackPtSumWithVertexandPatatrackSizeWithVertex.The reason is the following: to fill the aforementioned variables while looping on tracks, a match between the vertex associated to the track (*) is required by associating the observable idv (which is the vertex index related to the patatrack) to be the same of the sortInd (**) which is the vertex index sorted by the vertex observable ptv2 .
If you want to give a look to the vertex related observables, they are described here.
This match is not correct: indeed, the idv has to be compared with the vertex index.
(*) all tracks with vertex index -1 are not considered, since this means that they are not associated to any vertex
(**) the sortInd is of a subset of all vertices, since they are required to pass some quality cuts described in the function SelectGoodVertices. Also in this function there is a similar matching requirement and also in that case it has been changed.
In this PR, this bug is fixed.
PR validation:
This bug was spotted because while looping multiple times on GPU, results were not consistent among each other.
With the aforementioned change, some preliminar tests on GPU and CPU have been carried on, selecting specific bugged events and from those preliminar tests there are no more differences.
With CPUs there were no difference both before and after the bugfix. So integrating this PR CPU results should be kept unchanged (in terms of efficiencies and rates).