Migrate LHCb::TrackFitResult,Measurement,Node to Rec

• define ITrackFitResult as base class of TrackFitResult implementations
• resolve circular dependency between TrackEvent and LHCbTrackInterfaces by forward declaring classes in interfaces, and move UTHit to DigiEvent

must be applied in conjunction with Rec!1359 (merged)

mentioned in merge request Rec!1359 (merged)

added 1 commit

• 61a3c723 - Migrate Measurement implementations from Rec -> LHCb

Compare with previous version

changed the description

@wouter, @hkuinder: FYI -- moving all Measurement implementations into the same package will make it much easier to replace them with a single, concrete, measurement class.

Also, this is on top of Rec!1343 (merged), which consolidates some Measurement implementations such that they are much simplified, eg. VPMeasurement, UTLiteMeasurement and FTMeasurement do not contain any data other than in the Measurement base class. The only functionality they have beyond their base class is that VPMeasurement adds a method which queries its trajectories direction to figure out whether it is a 'X' or a 'Y' measurement -- and this is solely used in some monitoring code... So we could actually replace those three measurements with a single concrete class.

added 19 commits

• 61a3c723...dcc905da - 17 commits from branch master
• 5314fb11 - Tweak v2::Track
• ee7289af - Migrate Measurement implementations from Rec -> LHCb

Compare with previous version

added 1 commit

• 39521553 - Migrate Measurement implementations from Rec -> LHCb

Compare with previous version

added 1 commit

• 18b377e1 - Re-order Measurement data members

Compare with previous version

added 1 commit

• d4fbda7c - Re-order Measurement data members

Compare with previous version

@graven I agree that moving all "Measurement" classes in one place is a good idea as it makes code development easier, but I wonder if it wouldn't be better to isolate everything related to the track fit into another place.

Measurements and Nodes are only used by track fit and monitoring. They could all be accessed through 'TrackFitResult'. Even in the current code, almost every time TrackFitResult is used it is first cast to KalmanFitResult, because that's the only way to get access to 'FitNode', which is much more useful than Node. So, what I would propose is

• use TrackEvent for Track, State and TrackFitResult only
• remove access to Measurement and Node from TrackFitResult: just make it an abstract interface class, with basically empty interface. (e.g. completely useless: just a place to store a pointer)
• agree that implementations of a trackfit use a class derived from TrackFitResult to cache info related to the track fit (e.g. also Peter's TrackHit class)
• adapt existing code such that it casts TrackFitResult to KalmanFitResult to access Measurements. (This is easy, since it is practically already done)
• move all Measurement, Node, FitNode classes to TrackFitEvent, or even to TrackFitter, such that we can develop the new 'old' track fit from there

added 1 commit

• bbb5fb40 - remove unused removeFrom{Measurements,Nodes}

Compare with previous version

@graven You'll need to rebase, following the merge of !1672 (merged)

@wouter: I've (almost) done what you proposed (i.e. I've got LHCb sorted out, but not yet Rec(*)), but before committing that variant, let me point out the consequences:

• the 'bare-bones' TrackFitResult is called ITrackFitResult and only has a (virtual) destructor and a (pure virtual) clone method. The two (virtual) methods are required as a TrackFitResult is owned by the Track and it thus to know how to delete and clone it as part of its destructor and copy constructor. Note that adding an I in front to the name makes it more obvious that after getting it, it has to be cast -- just calling it TrackFitResult would be a bit tricky as that name currently means something else. But we can change the name later once everything is migrated)
• the nodes() and measurements() , nMeasurementsRemoved() and nMeasurements() accessors disappear from v1::Track' (not a real problem, just anything using them first has to grab a ITrackFitResult`, cast it, and then get them -- unless we add this to the 'data interface')
• the behaviour of closestState changes - it used to first look through the nodes (if available) and only then look for the states stored in the track. Now, it goes directly to the stored states. (this seems more consistent to me, but it is different)

So the only real problem (all others can be worked around) is the behavior change of v1::Track::closestState -- which I suspect is not a real problem, but I'd like to hear comments on before proceeding ;-)

(*) I need to find a way to resolve the circular dependency in Rec between Tr/TrackFitEvent and Tr/TrackKernel introduced by moving Node and Measurement into TrackFitEvent...

@graven Fantastic!

I agree with your proposal for closestState: We can always add another closestState method to KalmanFitResult.

I think that we cannot prevent that TrackKernel depends on TrackEvent. Removing its dependency on TrackFitEvent seems hard as well, unless we remove its constructor that takes a track or a set of nodes as input. (This seems okay: We could leave just the constructor from a range of states, and eventually put some global functions that create it from other sources.)

However, we can remove the dependency of TrackFitEvent on TrackKernel, at least for now: AFAIK OriginConstraint isn't used by anybody but me in some private code, so we could remove it. In the long turn, perhaps we should merge TrackKernel and TrackEvent?

@pkardos For your information. How does it interact with your refactoring of HLT1 ? I suppose it does not ?

I think originally TrackKernel was kept separate from TrackEvent to avoid adding to the event model additional dependencies, to the detector description in particular. But looking at the current content of TrackKernel:

• There is dependency to DetDesc declared in CMakeLists.txt, but it does not seem to be used anywhere
• There is a (undeclared) dependency on Relations: TrackCloneData.h uses Relations/Range.h to access a range of LHCbIDs - could it not use range_v3 instead?

Other than that, the only dependencies are to gsl, GaudiKernel, LHCbMath, LHCbKernel and TrackEvent, so I think this could be merged into TrackEvent straightaway (if we can get rid of the Relations dependency). Some stuff (e.g. `LineDifTraj') could even go to LHCbKernel.

I'm slowly getting there (looks like I've resolved the dependencies in Rec) but there is one thing that I'm realizing as I go along (and add lots of dynamic_cast<TrackFitResult*>(track.fitResult()): the only reason why one cannot have a pointer to an 'opaque' (ie. just a forward declared class, without definition) as member data seems to be due to 'ownership' -- in the above, I pointed out that the ITrackFitResult must have a destructor, and a clone method. Both of these are the direct consequence of ownership. If instead the ownership would lie elsewhere (eg. the event store!) one could just put an opaque pointer as data member, and only the 'user' of that pointer would have to include the full definition in order to do something useful with it.

Now I had already come to the conclusion that we should have composite objects (such as Tracks) without owning their 'payload' -- eg. there was the discussion that any new track should have a 'span' of hits, and not a 'vector' of hits, so that all hits (of a given type) of all tracks could reside in a single container (which is possible as they all are created at the same time, by the same algorithm, and have the same lifetime), and that would make eg. deleting the event much more efficient -- but it is now also clear that from a 'packaging' point of view this is better.

In the past this was typically not done, as someone could go 'behind your back' and change things stored in the TES (and strictly speaking, that is technically still possible...) so having composites using 'views' of TES data was risky, and it makes persistence more work.

Anyway, once I've made this work, I propose that a follow-up investigates storing TrackFitResult directly in the TES, and adopt the code to that, to see whether it simplifies things. (or if we want to support multiple independent types of fit result, some variant over a set of opaque pointer types in the Track (v1) )

added 5 commits

• bbb5fb40...c173ce9d - 2 commits from branch master
• 058055e8 - Re-order Measurement data members
• fa87726e - remove unused removeFrom{Measurements,Nodes}
• af456b6c - Migrate Measurement,Node,TrackFitResult from LHCb->Rec

Compare with previous version

changed title from WIP: Migrate LHCb::Measurement implementations to LHCb to WIP: Migrate LHCb::TrackFitResult,Measurement,Node to Rec