Skip to content

Conversation

@valeriadamante
Copy link
Contributor

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 : PatatrackPtSumWithVertex and PatatrackSizeWithVertex.
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).

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is the backport of the PR cmssw-37291

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 28, 2022

A new Pull Request was created by @valeriadamante (Valeria D'Amante) for CMSSW_12_3_X.

It involves the following packages:

  • RecoTauTag/HLTProducers (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@silviodonato, @mbluj, @azotz this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Martin-Grunewald
Copy link
Contributor

please test

@missirol
Copy link
Contributor

backport of #37291

@valeriadamante , please add "L2TauTagNNProducer" to the title of this PR.

@missirol
Copy link
Contributor

type bugfix

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-384b30/23443/summary.html
COMMIT: 73d1eea
CMSSW: CMSSW_12_3_X_2022-03-28-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37372/23443/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695161
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3695137
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Mar 28, 2022

+hlt

@valeriadamante , please don't forget the following

add "L2TauTagNNProducer" to the title of this PR.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_3_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_4_X is complete. 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)

@valeriadamante valeriadamante changed the title backport of bug fix in vertex selection backport of bug fix in vertex selection for the L2TauTagNNProducer Mar 28, 2022
@valeriadamante valeriadamante changed the title backport of bug fix in vertex selection for the L2TauTagNNProducer backport of bugfix in vertex selection for the L2TauTagNNProducer Mar 28, 2022
@qliphy
Copy link
Contributor

qliphy commented Mar 29, 2022

+1

@cmsbuild cmsbuild merged commit f3d8a55 into cms-sw:CMSSW_12_3_X Mar 29, 2022
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.

5 participants