DerivationFrameworkTau: fix duplicate tau removal
Hello,
We have a weakness in the RAWtoALL tau reconstruction that can produce taus with same (eta,phi) although they are built from different seed jets. A protection was implemented in AOD->DAOD, but there was an issue with vector element removal while iterating over the vector: the vector size changes while iterating. If we have 3 duplicate taus consecutive in the vector (should be extremely rare), the existing protection would retain 2 taus. To fix this, the loop index is incremented only when no duplicate is found.
Tagging @ademaria .
Cheers, Bertrand
Merge request reports
Activity
added Derivation Tau main review-pending-level-1 labels
added bugfix label
Athena externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
added review-approved label and removed review-pending-level-1 label
Resolved by Bertrand Martin Dit LatourHi @martindl ,
I think the duplicate removal could be safely implemented using only iterators:
std::vector::erase(..)returns an iterator the element following the deleted one. So you could do something likefor (it=v.begin();it!=v.end();) { if (eta/phi match) it=v.erase(it); else ++it; }- Walter
- Last reply by Bertrand Martin Dit Latour
added review-user-action-required label
removed review-approved label
added review-pending-level-1 label and removed review-user-action-required label
-
Athena externals cmake make tests 
Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Unrelated ATLASG-2843 failure
-
Athena externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
added review-approved label and removed review-pending-level-1 label
mentioned in commit 90786069
83 83 } 84 84 85 86 if(tausToKeep.size() >0) { 87 for(size_t i=0; i < tausToKeep.size()-1; i++){ 88 for (size_t j=i+1; j < tausToKeep.size(); j++){ 89 if( (tausToKeep.at(i)->p4().DeltaR(tausToKeep.at(j)->p4())) < 0.01){ 90 ATH_MSG_WARNING("Found duplicated tau with eta " << tausToKeep.at(j)->eta() << " phi " << tausToKeep.at(j)->phi() << " pt " << tausToKeep.at(j)->pt() << ". Removing it ..."); 91 tausToKeep.erase( tausToKeep.begin()+j); 92 } 85 // protection against duplicate taus -- built from different seed jets, but end up having same (eta,phi) 86 for (auto it=tausToKeep.begin(); it!=tausToKeep.end(); ++it) { 87 for (auto it2=std::next(it); it2!=tausToKeep.end(); ) { 88 if ((*it)->p4().DeltaR((*it2)->p4()) < 0.01) { 89 ATH_MSG_WARNING("Found duplicate tau with eta=" << (*it2)->eta() << " phi=" << (*it2)->phi() << " pt=" << (*it2)->pt() << ". Removing it, keep tau with pt=" << (*it)->pt()); 90 it2 = tausToKeep.erase(it2); -
@martindl this is flagged by cppcheck in !77034 (merged).
There is no guarantee that your original iterator
itis still valid after callingeraseon the container. Please rework the code.Edited by Frank Winklmeier -
My comment !77030 (comment 8902155) referred only to the inner loop.
-
How about:
for(size_t i=0; i < tausToKeep.size()-1; i++){ const auto* aTau=tausToKeep[i]; std::remove_if(tausToKeep.begin()+i,tausToKeep.end(),[aTau](const xAOD::TauJet* bTau) {return aTau->p4().DeltaR(bTau->p4()) < 0.01;}); }Edited by Walter Lampl -
@martindl could you please address this? It's blocking the wider cppcheck deployment.
Hello @fwinkl, we are following up with this here !77162 (merged)
mentioned in merge request !77162 (merged)
