SOA Track Fix

Fix conflicts between SOA Track MR !2920 (merged). Run concurrent with Rec!2408 (merged) and Phys!925 (closed).

Do we plan to use this also for Run1/2 at some point? Otherwise I would remove all references to Run1/2 detectors / track types. (Same for VeloR).

This is to allow v1::Track/v2::Track to PrFittedGenericTrack conversions (the old versions of the tracks define all those state locations). I don't think locations like VeloR are used by old track versions either, but I would keep it as it is for now in order to have full compatibility. As soon as we have done the necessary checks with the ThOr functors and we are sure we don't need to convert among track types any more I agree we can remove them.

I don't quite understand. Nothing in Run3 will make VeloR or TT tracks, so I cannot see any scenario why this would be needed. And if unexpectedly it would still turn up somewhere, at least it would crash now :)

The idea is to preserve the direct mapping of v1::Track and v2::Track objects to the new track type. We can drop such functionality here and raise exceptions in such specific cases (like trying to create a VeloR track), but I was inclined to do it step-by-step.

I have done the appropriate modifications. TT and VeloR are no longer supported.

it would be nicer if set would be the sole place where all this indexing has to be done, eg. it would support writing the above as:

    outTrack.template field<Tag::StateCov>( L ).set( cov );

instead.

That way, all that indexing only happens in one location... (but/and it forces the order of elements, which depending on your point of view could be good or bad -- but I think it would be good ;-)

there is a macro ASSUME defined here which in debug builds will actually check whether the assumption is correct or not -- so:

    ASSUME( false );

which at least in debug builds will report that an assertion failed, instead of 'undefined behavior'

Also, nicer would be:

    for ( auto [ idx, i ] : LHCb::range::enumerate( members_of<StateLocation>() ) ) {
      if ( i == s ) return idx;
    }
    ASSUME( false );

Anyway, a lot faster would be to avoid having to loop at all:

   constexpr auto index( StateLocation s ) {
      ASSUME( s != StateLocation::Unknown );
      return s-1;
   }

and make sure that ends up as the correct answer 'by construction'...

Sounds good. Concerning the loop, it is not quite safe to do a static_cast since enumeration types could correspond to non-consecutive numbers. Although since the definition of StateLocation is right above maybe it can be done.

As defined, the numbers are consecutive -- and if you're paranoid, you could explicitly assign them (and make Unknown to be huge, or '-1', or ... so that no subtraction is needed). And I would expect this to be 'very hot' code, so avoiding the loop here should matter.

I was hoping that the compiler could optimize it since the array is static constexpr, but I just checked that this is not always the case. Let's keep it simple then.

that is a necessary, but as you found out, not sufficient condition -- as it depends on whether the StateLocation argument is (also) a compile-time known constant or not, which depends on the caller...

I was actually wondering if the compiler could reduce it to a switch/case-like statement, of course, since the addition operation is not used as constexpr (this is why we accidentally ran into an undefined behaviour before, which suggested these changes), the checks need to be done a runtime.

assigned to @agilman and @jbaptist

mentioned in merge request Phys!925 (closed)

mentioned in merge request Rec!2408 (merged)

• [2021-04-21 13:18] Validation started with lhcb-master-mr#2228

closed

unassigned @jbaptist and @agilman

mentioned in issue Moore#272 (closed)