I would have expected continue rather than break (assuming this is the n-hits of one given track). No?

this WAS the bug.
it is a guard
see for instance in
https://cmssdt.cern.ch/dxr/CMSSW/source/RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinder.cc#30

it will not be required anymore once #37281 is merged

Okay, thanks for clarifying.

The upper limit from maxVtx_ has been removed. Either re-instate it, or remove the corresponding configuration parameter (and class data member).

I'm generally confused about the role of vtxGood, but obviously I'm not the expert here.

Is the order of the elements in vtxGood important?

• If not, why do you need to use the sorting from the input vertices (soa.sortInd[j])?
• If yes, since the sorting from the input vertices is used, shouldn't one use the f.o.m. that was used for that sorting? Is that the purpose of pTSquaredSum (to match exactly the f.o.m. used for the original sorting)? If so, wouldn't that value already be available in soa.ptv2[sortInd[j]]?

Is vtxGood meant to mimic the "trimmed" pixel vertices? If yes, why not simply use those as (additional) input to this producer, without having to recompute all this and having to keep this whole source code aligned with the pixel reco?

Is the order of the elements in vtxGood important?

• If not, why do you need to use the sorting from the input vertices (soa.sortInd[j])?

No, the order is not important. patavtx_soa.sotrInd[vtx_idx] is needed to make track-vertex association, because patavtx_soa.idv[trk_idx] refers to sortInd instead of vtx_idx.

Is vtxGood meant to mimic the "trimmed" pixel vertices? If yes, why not simply use those as (additional) input to this producer, without having to recompute all this and having to keep this whole source code aligned with the pixel reco?

In L2TauTag producer we prefer to avoid the use of legacy formats to eliminate unnecessary overhead from conversion and creation of the new collection. While indeed in the current version, the vertex selection mimics "trimmed" vertex producer logic, we plan to change the selection for future training (e.g. use soa.ptv2, review cuts applied to trkGood and vtxGood).

No, the order is not important. [..]

In this particular loop, I don't see .idv, so it's not clear to me how [1] would differ from [2] if the order of vtxGood is not important.

[1]

    for (int j = nv - 1; j >= 0; --j) {
      auto vtx_idx = patavtx_soa.sortInd[j];
      assert(vtx_idx < nv);
      if (nTrkAssociated[vtx_idx] >= 2 && pTSquaredSum[vtx_idx] >= minFOM_fromFrac &&
          pTSquaredSum[vtx_idx] > minSumPt2_) {
        vtxGood.push_back(vtx_idx);
      }
    }

[2]

    for (int vtx_idx = nv - 1; vtx_idx >= 0; --vtx_idx) {
      if (nTrkAssociated[vtx_idx] >= 2 && pTSquaredSum[vtx_idx] >= minFOM_fromFrac &&
          pTSquaredSum[vtx_idx] > minSumPt2_) {
        vtxGood.push_back(vtx_idx);
      }
    }

Sorry, I've misunderstood your original comment. Yes, in this particular loop [1] and [2] will result in equivalent NN inputs. So let's use [2] since it is simpler.

Hm.. actually, as @valeriadamante pointed me out, after adding back maxVtx_ [1] and [2] will no longer be equivalent because if the number of potentially good vertices is greater than maxVtx_, we would want to keep the most energetic ones. However, the order in which they will be stored in vtxGood is not important.

Right, I see. In principle, one could also fill vtxGood then use if (vtxGood.size > maxVtx_) vtxGood.resize(maxVtx_) afterwards (without having the if (vtxGood.size > maxVtx_) check inside the for loop). I don't know what's better and faster, it might depend on the typical value of vtxGood.size and maxVtx_. You are free to use the implementation you prefer, of course.

Can one actually have nTrkAssociated < 2 for a vertex? If not, could/should nTrkAssociated be removed altogether? If not, should the threshold be configurable and not hard-coded to 2?

@valeriadamante , I see you updated the PR, but I'd like to have a clarification on this question before restarting the tests.

perhaps @VinInn can comment on this. For L2TauTag we copied this condition from here:

Okay, understood. If further changes are eventually made to this PR (or in a future one), you could consider adding comments on where these different cuts are taken from, and what they need to be kept aligned with.