RooUtil: in-place GetTracks/GetCaloClusters selection can erase the kept entries (remove_if tail)

[oksuzian]

In-place selection in RooUtil, GetTracks(cut, true) / SelectTracks() and GetCaloClusters(cut, true), can erase the entries that pass the cut from the backing branch vectors and keep the rejected ones.

Checked against EventNtuple main at 5db1a2a, in rooutil/inc/Event.hh.

Mechanism

GetTracks(cut, true) (Event.hh:276-312):

auto newEnd = std::remove_if(tracks.begin(), tracks.end(), [cut](Track& track) { return !cut(track); });
for (auto i_track = newEnd; i_track != tracks.end(); ++i_track)   // tail = "the rejected ones"
  for (size_t i_trk = 0; i_trk < trk->size(); ++i_trk)
    if (&(trk->at(i_trk)) == i_track->trk) trks_to_remove.emplace_back(i_trk);
... trk->erase(...), trkmc->erase(...), trkhits->erase(...), ...

std::remove_if does not move the rejected elements to the tail. It assigns the kept elements forward, and the elements in [newEnd, end) are left in a valid but unspecified state. A Track wrapper is copy-assigned, so in practice the tail holds whatever was there before the assignment, which is often a copy of a kept track.

Take two tracks, [reject, keep]:

1. remove_if assigns tracks[0] = tracks[1]. The vector is now [keep, keep], with newEnd at index 1.
2. The tail entry tracks[1] still points at the kept trk[1], so trk[1] is flagged for removal.
3. The loop erases the kept track from trk, trkmc, trkhits, trksegs and the rest. The rejected track stays in every branch.
4. The surviving wrapper's trk pointer now points one past the end of trk.

GetCaloClusters(cut, true) (:371-389) has the same structure. It applies to caloclusters.

Reproduction

This is a standalone program with the same logic, not the RooUtil code itself:

std::vector<Info> trk = {{1 /*reject*/}, {2 /*keep*/}};
std::vector<Wrap> tracks = {{&trk[0]}, {&trk[1]}};
auto newEnd = std::remove_if(tracks.begin(), tracks.end(), [](Wrap& w){ return w.p->id != 2; });
// ... same tail scan and erase as Event.hh ...

It prints:

backing trk after selection: size=1 id=1 (expected id=2)
wrapper points at &trk[1] (past end): yes

The outcome depends on the order of the entries. With [keep, reject] the tail really is the rejected entry, and the result is correct. So this only shows up in events where a rejected entry comes before a kept one.

Impact

• rooutil/examples/CreateNtuple.C:25 calls event.SelectTracks(is_e_minus) and then writes the event out. So a skim made this way can contain the rejected tracks, together with their trkhits/trkmc/trksegs rows, and lose the ones that were kept.
• In the same event, the Track wrappers returned to the caller dereference a pointer past the end of the vector.
• The non-in-place overloads, GetTracks(cut) and GetCaloClusters(cut), are not affected.

Suggested fix

#408 already fixes this for time clusters. Its approach: evaluate the cut once into a keep flag per backing index, erase the indices where !keep in reverse order, and rebuild the wrappers. Doing the same in GetTracks(cut, true) and GetCaloClusters(cut, true) would remove both instances. An ordering test with a rejected entry before a kept one, like the one in #408's validation/test_collection_discovery.C, would cover it.

Found while reviewing #408.

[michaelmackenzie]

I tested having Sonnet try to solve this, since it seemed simple enough:
#417