Analyzing the relevant bit of code a bit more, it becomes clear that the following snippet:

std::sort( temp.begin(), temp.end(), Tf::increasingByProjection<PatForwardHit>() );

[ debug stuff not shown ]

PatFwdHits::iterator itP, itB, itE, itF; for ( itP = temp.begin(); temp.end() - minYPlanes >= itP; ++itP ) { itE = itP + minYPlanes -1; double spread = maxSpread; if ((*itP)->hit()->type() == Tf::RegionID::OT) spread += 1.5; // OT drift ambiguities...

if( UNLIKELY( msgLevel(MSG::VERBOSE) ) ) { verbose() << format( " first %8.2f +minXPlanes -> %8.2f (diff: %8.2f) Spread %6.2f ", (*itP)->projection(), (*itE)->projection(), (*itE)->projection() - (*itP)->projection(), spread ); }

//== If not enough hits in the maximum spread, skip if ( spread < (*itE)->projection() - (*itP)->projection() ) { while( spread < (*itE)->projection() - (*itP)->projection() ) itP++; --itP; // as there will be a ++ in the loop ! if( UNLIKELY( msgLevel(MSG::VERBOSE) ) ) verbose() << " not enough planes in spread" << endmsg; continue; }

//== Add all hits inside the maximum spread. If not enough planes, restart while ( itE != temp.end() && spread > (*itE)->projection() - (*itP)->projection() ) itE++;

// do stuff...

is ‘just’ iterating over a ‘sliding window’, and could be replaced by generic code. (there is the complication that the window size depends a bit on the ‘type’ of object in the list, but that isn’t so bad)

I think the code above could be written as:

std::sort( temp.begin(), temp.end(), [](const PatFwdHit* l, const PatFwdHit* r) { return l->projection()<r->projection(); } ); for ( auto window : windows( temp, minYPlanes, [&](const PatFwdHit* first, const PatFwdHit* last){ auto spread = maxSpread; if (first->hit()->type()== Tf::RegionID::OT) spread+=1.5; return first->projection() < last->projection()+spread; } ) { itP = window.first; itE = window.second;

// do stuff...

}

Maybe I have some time at the end of week to implement something like the above cleanly….

— Gerhard

On 11 Feb 2014, at 00:47, Gerhard Raven gerhard.raven@nikhef.nl wrote:

Note: this is a complete tangent to what we discussed today…

Looking at the code for PatForwardTool::fillStereoList, I see that the ‘temp' list of hits is sorted at some point:

std::sort( temp.begin(), temp.end(), Tf::increasingByProjection<PatForwardHit>() );

and a bitter further down in the code, there are then various loop like this (where itP is an iterator into temp, with itE > itP):

//== Add all hits inside the maximum spread. If not enough planes, restart while ( itE != temp.end() && spread > (*itE)->projection() - (*itP)->projection() ) ++itE;

which can (I’m pretty sure) be rewritten as:

itE = std::find_if( itE, temp.end(), [&](const PatFwdHit* h) { return h->projection() > (*itP)->projection() + spread ; } );

but then I started to think that the sorted-by-projection list is also sorted by the criteria in the above find_if, and hence the std::find_if can be replaced by std::binary_sort… and I suspect that in a few places here actually a std::equal_range is what is really going on. (then again, might be that the # of items skipped over in find_if may be less than log(N))

Anyway, this code really would benefit from someone transforming it into code using C++11 and generic STL algorithms, and by making it more readable so one can recognize what’s really going on…

-- Gerhard