Christos Anastopoulos authored 4 years ago and Adam Edward Barton committed 4 years ago

TrackSummary Tool, make Event context aware i.e add ability to pass EventContext from the caller and then propagate down the call chain

TrkGeometry Layers: Remove unneeded virtual methods.Also mark methods as final where applicable/possible

Remove unneeded / undefined EIGEN_INCLUDE_DIRS.

Stephen Nicholas Swatman authored 4 years ago and Adam Edward Barton committed 4 years ago

Currently, we're storing the charge value of the pattern track
parameters in a cached value, but this is not really necessary.
Computing the charge is so trivial and fast (from the q/p value) that
there really is no reason to cache this. We use the lazy binding
behaviour of the charge method to simply recompute on the fly.

Implement mungeZeroQOverP in terms of updateParameters , and SurfaceGetCovariance via a non-const method overload

The track extrapolation packages TrkExTools and TrkExUtils are related
to the pattern track parameters, but neither of these packages are
properly linked against the pattern track parameter packages. This
commit ensures that both of these packages are linked publicly against
the pattern parameters. This was not giving problems before - I'm not
exactly sure why, but I ran into linking problems with some changes to
the pattern code.

In my previous work to migrate the GlobalChi2Fitter to smart pointers, I
introduced a minor bug-prone bit of code where a smart pointer may be
used after being moved. While this has not been disasterous, it is still
erroneous behaviour that clang-tidy picks up on, as described in
ATLASRECTS-5738. This commit resolves the issue by keeping a non-unique
observer pointer to the object, and using that instead of the unique
handle which may be moved. This is mostly a quick fix, the method
containing this code is extremely complex and needs some refactoring.

Inconsistent function declarations.

Spurious copy in for loop.

 - Missing override keywords.
 

In the current implementation of the track parameters, the position and
momentum vectors are returned by reference. This is enforced at an
interface level by the `ParametersBase` class. Unfortunately this is not
necessarily good interface design, for several reasons:

Firstly, this interface design forces certain design choices on the
implementing classes. That is to say, the implementing classes need to
store some vector internally in order for it to be possible for them to
return them by reference. This robs the implementing classes of the
freedom to design their internals in the most efficient possible way.

Secondly, it breaks encapsulation of the class. The internals of the
track parameters class should be hidden from the outside world, and this
concept of information hiding is integral to good object-oriented
programming. However, returning references to members of the class
breaks this rule. Returning the references as const alleviates this
problem somewhat, but the implementation is still far from ideal.

Thirdly, there is a potential lifetime issue with this design, as a
reference to the internal vectors of the track parameters object can
outlive the parameters object which they point to, which can lead to
memory unsafety. Unless a Rust-like reference lifetime is implemented on
a code level (because it is not supported by the C++ language standard)
there is virtually no way to go about this safely.

Finally, there are thread-safety concerns with this design, as it is
possible for one thread to update the internals of the object while
another thread has a seemingly const reference to its internal data.
This is not supposed to happen in the Athena code, but it's hard to
enforce such requirements.

For more information on this design anti-pattern I recommend the book
"Effective C++" by Scott Meyers, specifically item 28 of chapter 5,
"Avoid returning handles to object internals."

The solution proposed in this commit is to move away from the
return-by-reference design, towards a return-by-value design. Returning
a copy instead of a reference to internal data resolves all the
previously mentioned concerns with the existing design, and should not
incur any significant performance hits.

At first it might seem like replacing this interface would be a
Herculean effort because these methods are used in so many different
places, but there is a saving grace that means that this change might be
simpler than expected. Indeed, the existing implementation uses const
references, which means that we can exploit the fact that temporaries
(and other rvalues) can be implicitly bound to const references to
extend their lifetime. That is to say, any existing code that relies on
this code returning const references should be compatible on a source
code level (not on an API level) with the new code returning these
vectors by value.

There are some cases in which the references returned by the track
parameter position and momentum methods are returned by reference for a
second time. Given the fact that we're attempting to move the
aforementioned methods to return by value, this means that these return
by reference methods would become memory unsafe. We change these methods
to be return by value themselves to resolve this issue.

This commit in particular resolves the issues in the Tracking realm.

Disable the Tool auto-retrievial introduced in Gaudi v34r1 for
`ActsTrackingGeometrySvc`.

GSF : KL Gaussian Mixture reduction , use an array to keep track of the merged components . Add/clarify a few more comments