Make TrackSummaryTool work with existing track summaries

As is stands right now, the Trk::TrackSummaryTool will completely forego creating a track summary for a track if one already exists, even if the existing one is only partially filled. This works well under the assumption that tracks are only given a summary by the Trk::TrackSummaryTool, but this is a model we're moving away from. Therefore, we need to modify the Trk::TrackSummaryTool not to skip, but to update and fill gaps in existing track summaries. This will prepare us for receiving tracks with summaries from, for example, track fitters. There is a possible performance pitfall here where we calculate values twice, but this does not occur in practice.

This merge request affects 2 packages:

• Tracking/TrkEvent/TrkTrackSummary
• Tracking/TrkTools/TrkTrackSummaryTool

Adding @amorley as watcher

added Tracking master review-pending-level-1 labels

Hey , not that I fundamentally disagree but can you give a bit details ? I do not object on the basis of the idea. But as I told to @goblirsc and @npetters at some stage , I would like to understand what you want to actually do.

In the one case we actually do this we call something like

/** method which can be used to update a refitted track and add a summary to
   * it, without doing shard hit/ or hole search. Adds a summary to a track and
   * then retrieve it from it without the need to clone. */
  virtual void updateRefittedTrack(Track& track) const override
  {
    updateTrackNoHoleSearch(track, nullptr);
  }

I think no fitter returns anymore const Trk::Track* anymore. So all summaries from fitters should be "updatable" no?

My issue typically has not been the create other than obvious the public create methods that update const tracks and remove bare ptr should all go ... https://gitlab.cern.ch/atlas/athena/-/blob/805ad8e1981e8cce01582f8f96ac17bc6b8fbec2/Tracking/TrkTools/TrkTrackSummaryTool/TrkTrackSummaryTool/TrackSummaryTool.h#L63

But that we do not have nice/polished update methods , in the other hand do not know how much we want to hack the tool even more. The underlying question and hackery is due to to be deprecated methods. If we go aways from const Track as results arguements to pass , then we can have kind of true updates.

Anyhow , as I said, because we suffered in the updateRefittedTrack more that I wanted, I would love to understand exactly the plan

CI Result FAILURE (hash 805ad8e1)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 20395]

What I mean is basically look here https://gitlab.cern.ch/atlas/athena/-/blob/master/Tracking/TrkTools/TrkToolInterfaces/TrkToolInterfaces/ITrackSummaryTool.h and let me add @sroe .

For the case of a fitter we can always update no? The fitter does not return const tracks no, so is Group C below?

• We have group A deprecated

virtual const Trk::TrackSummary* createSummary
  ATLAS_NOT_THREAD_SAFE(const Track& track,
                        bool onlyUpdateTrack = false) const = 0;

  /** create a summary object of passed track without doing the tedious hole
     search. Same comments as for createSummary apply here, of course, too. */
  virtual const Trk::TrackSummary* createSummaryNoHoleSearch
  ATLAS_NOT_THREAD_SAFE(const Track& track) const = 0;

• Group B , we can not modify the input. Kept for alternative to Group A. It is clone or do all for some definition of compatibility.

virtual std::unique_ptr<Trk::TrackSummary> summary(
    const Track& track) const = 0;

  /** create a summary object of passed track without doing the tedious hole
   search. If the track has a summary already a clone is returned back.*/
  virtual std::unique_ptr<Trk::TrackSummary> summaryNoHoleSearch(
    const Track& track) const = 0;

• Group C update methods.

     This can be used to add a summary to a track and then retrieve it from it
     without the need to clone. */
  virtual void updateTrack(Track& track) const = 0;

  /** method which can be used to update a refitted track and add a summary to
   * it, without doing shard hit/ or hole search. Adds a summary to a track and
   * then retrieve it from it without the need to clone. */
  virtual void updateRefittedTrack(Track& track) const = 0;

  /** method to update the shared hit content only, this is optimised for track
   * collection merging. */
  virtual void updateSharedHitCount(Track& track) const = 0;

  /** method to update additional information (PID,shared hits, dEdX), this is
   * optimised for track collection merging. */
  virtual void updateAdditionalInfo(Track& track) const = 0;

• So I assume Group A will go. Group C is what we kind of want when we really want to update. GroupB we can discuss is if needs to also "augment" before cloning. But groupB was there for clients that need kind of "cloning" all or nothing behaviour.

• The tool does not work unders the assumption that it is the only provider of summary . The tool can handle/should update of an existing summary of a non const track as returned from a fitter. For something like the output of GSF we use the update as this makes much more sense.

*Fundamentally, I do not disagree we should furhter clean up. *

But also not sure what is the exact plan .

• For example for me the first issue on something like this https://acode-browser1.usatlas.bnl.gov/lxr/source/athena/Tracking/TrkTools/TrkAmbiguityProcessor/src/TrackScoringTool.cxx#0078

3 Trk::TrackScoringTool::score( const Track& track, const bool suppressHoleSearch ) const{
0074     const TrackSummary* summary = nullptr;
0075     if (suppressHoleSearch)
0076       summary = m_trkSummaryTool->createSummaryNoHoleSearch(track);
0077     else
0078       summary = m_trkSummaryTool->createSummary(track);

is that it hits the non safe methods

• The 2nd issue is that the const does not come from the "fitter". fitter return perfect updatable tracks.

• Obviously since you const_cast anyhow, you could also use the "update", no need to alter the clone.

I would prefer if we 1st understand what we are really after here in terms of update vs just fill everything you can /clone.

If we want to update a summary of non-const track as returned by a fitter this is perfectly possible. We can update non-const, and we can clone existing const. The worse is const_cast and then clone or do full which is only one place in the full code base that does it the TrackScoringTool.

Actually, I will do an MR to remove the ATLAS_NOT_THREAD_SAFE methods and just make explicit what the TrackScoringTool actually does, and then perhaps it will be cleaner what I find confusing here ...

To be a bit concrete on my comments above and as is not that fundamentally different. Here are my kind of ideas

christos/athena@cd2b4748

Basically is like introducing something like fillSummary (just takes something and fills it)

https://gitlab.cern.ch/christos/athena/-/blob/cd2b474814201f116ad31c4c026aaaf88eeb1a37/Tracking/TrkTools/TrkTrackSummaryTool/src/TrackSummaryTool.cxx#L175

Then create summary looks like https://gitlab.cern.ch/christos/athena/-/blob/cd2b474814201f116ad31c4c026aaaf88eeb1a37/Tracking/TrkTools/TrkTrackSummaryTool/src/TrackSummaryTool.cxx#L326

And computeAndReplace becomes computesAndUpdate

https://gitlab.cern.ch/christos/athena/-/blob/cd2b474814201f116ad31c4c026aaaf88eeb1a37/Tracking/TrkTools/TrkTrackSummaryTool/src/TrackSummaryTool.cxx#L136

So now a call like https://gitlab.cern.ch/christos/athena/-/blob/cd2b474814201f116ad31c4c026aaaf88eeb1a37/Tracking/TrkTools/TrkTrackSummaryTool/src/TrackSummaryTool.cxx#L352

will not really do a clone if a summary is there, it will just fill something based on options.

Is not fundamentally different to what you try perhaps... in the end is a similar effect. But rather than try to wrap too mich the logic of create, I just add a fill and you then create and fill (if you want) , or you have an existing to update, and fill the it . Not creating new clone if you do not have to i.e if a summary with hole info is attached....

Jenkins please retry a build

build area is cleaned as the previous build was for the MR labeled as full-build

This merge request affects 2 packages:

• Tracking/TrkEvent/TrkTrackSummary
• Tracking/TrkTools/TrkTrackSummaryTool

Adding @amorley as watcher

CI Result SUCCESS (hash 805ad8e1)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 1
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 20447]

Changes look fine. Waiting on resolution of discussion above. The compilation warning appears to be unrelated to these changes, so once the discussion concludes we can probably approve. (L1 shifter)

added review-user-action-required label and removed review-pending-level-1 label

resolved all threads

I resolved the threads , @sswatman I can do the thing I want on top if you think you want to avoid it. But really I would like to also avoid clones on updates if not 100% needed

resolved all threads

added 209 commits

• 805ad8e1...5cadef22 - 207 commits from branch atlas:master
• a71f6cc2 - Make TrackSummaryTool a friend of TrackSummary
• ab85068e - Make TrackSummaryTool work with existing summaries

Compare with previous version

Merge conflict has been resolved, and all threads have also been resolved.

This merge request affects 2 packages:

• Tracking/TrkEvent/TrkTrackSummary
• Tracking/TrkTools/TrkTrackSummaryTool

Adding @amorley as watcher

added review-pending-level-1 label and removed review-user-action-required label

CI Result SUCCESS (hash ab85068e)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 1
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 20613]