it is already defined in the TrackTPC.h, better to no redefine it.

Ok, I will clean it up

I think it is better to use TrackExtraInfo extraInfoHolder; local to the loop, then you don't need all these reinitializations

Yes, it is a better way indeed

The types of different track will be growing, isn't it better to use a "track-sources" option like

Thank you for this advice, I didn't think about it :)

and then, instead of having separate block (with mostly repeating code) for every type of barrel track, you can have a single block

auto contributorsGID = recoData.getSingleDetectorRefs(src);
if (contributorsGID[GIndex::Source::ITS].isIndexSet()) {
 // process ITS part from contributorsGID[GIndex::ITS]
}
if (contributorsGID[GIndex::Source::TPC].isIndexSet()) {
 // process TPC part from contributorsGID[GIndex::TPC]
}
// etc.

I don't see where do you save TPC Nclusters? BTW, with the current TPC tracking one can have >1 cluster on a given padrow. For the calculation of the tpcChi2NCl it is indeed better to use tpcOrig.getNClusters(), but as Nclusters stored in the AOD I would use actual number of contributing padrows, like it is calculated in the snippet I sent you before.

@shahor02, I thought we agreed on filling number of shared clusters for now, but maybe I misunderstood. Where should I put found and crossed for now then? The corresponding columns are dynamic

What I wrote before is that tpcNClsFindable is not available in run3 reco output with the same definition as in run2, so better to use a placeholder at the moment. For instance, you can use instead tpcNClsCrossedRows in order to define the differences currently present in the AOD.

we discussed during the Monday meeting the way TOF info is stored, at the moment please store the extraInfoHolder.tofExpMom as in run2 AOD conversion:
https://github.com/alisw/AliPhysics/blob/820236aa6f0149a27d5204b64bb52620772df90f/RUN3/AliAnalysisTaskAO2Dconverter.cxx#L1657-L1665

Corresponding methods were implemented in

Thank you for the snippet. I will try adding this column

There are 4 variables to fill, I would use:

extraInfoHolder.tpcNClsFindable = tpcOrig.getNClusters();
extraInfoHolder.tpcNClsFindableMinusFound = tpcOrig.getNClusters() - found;
extraInfoHolder.tpcNClsFindableMinusCrossedRows = tpcOrig.getNClusters() - crossed;
extraInfoHolder.tpcNClsShared = shared;

bearing in mind that such a definition of tpcNClsFindable is wrong and in some cases the tpcOrig.getNClusters() - crossed can be <0. Later we should find a proper substitute for the tpcNClsFindable.
As I wrote before, the tpcOrig.getNClusters() itself is not a good variable, since in the new reconstruction if the fitter considers that the 2 clusters on the same padrow may come in fact from the same split cluster, it will count (and fit) both of them, thus the getNClusters() may actually exceed the number of crossed rows.

Done, thank you

This does not look correct:

1. the extraInfoHolder.tofSignal should be the the measured time of flight rather than the energy. As in the current o2::dataformats::MatchInfoTOF there is no getSignal() method, the best if @noferini hints how it should be extracted (I think corresponding data member should be added).
   The extraInfoHolder.tofExpMom should be calculated as mass * expBeta / sqrt(1. - expBeta*expBeta) , the expBeta will be correct provided the expSig is fixed. I've asked @njacazio to verify this by it seems he is offline.

PS: details of TOF parameters are here: https://indico.cern.ch/event/1010843/contributions/4242241/attachments/2193866/3708654/jacazio_2021-02-22_O2PID.pdf

I see. I will be waiting for the response then

The TOF time is not propagated in the MatchingInfo but it can be recovever from the cluster (tofclusters.root) with position
tofMatch.getTOFClIndex()
If you think it is better to propagate this infromation in the matching info I can add the data member and then the proper method. Do you need it?

By accepting this
nburmaso#1
you could simply do
extraInfoHolder.tofSignal = tofMatch.getSignal();

Please, note that the interaction time is not subtracted (you get a time inside the timeframe)

Hi @shahor02. I have fixed tofExpMom calculation, could you please check if it's ok?

I see a problem here: in the Run2 -> Run3 AOD conversion we will a truncated float value,
https://github.com/alisw/AliPhysics/blob/bb0e58318cd9d7fc90d659a724643b0f2b1c623e/RUN3/AliAnalysisTaskAO2Dconverter.cxx#L1650
to https://github.com/alisw/AliPhysics/blob/bb0e58318cd9d7fc90d659a724643b0f2b1c623e/RUN3/AliAnalysisTaskAO2Dconverter.h#L326.
This should be fine for Run2 AliESDtrack, where the time is defined wrt to the trigger, but, as @noferini explained, in Run3, with the time defined wrt the TF start, we should use double. And, we cannot redefine this time wrt some other reference (e.g. interaction time) since in general the track is not guaranteed to be related to any reference.
I would propose to change float expSig and extraInfoHolder.tofSignal to double and add it on

I have a comment regarding propagation of tof time to aod: float or double?
Since tof time is filled in a method called with the collisionID as argument I would expect that at this stage you already know at least what is the BC candidate. In such a case you should be able to subtract a collision time with a precision depending on what you have. I think it doesn't make sense propagate to aod tof time referred to the the TF (even if I didn't check the logic of AOD producer).

@noferini see my comment below: for unassigned tracks the collisionID and interactionTime are dummy parameters.
But even for assigned tracks the current interactionTime is not guaranteed to be even BC-precise (and, btw, it is rounded to ns):

I do not think this fully solves it.

For assigned tracks, we could store wrt to track.collision().bc()

But as you point out unassigned tracks, do not have a collision or bc. Storing with respect of TF start also does not work, because we do not store the TF start. As Ruben pointed out earlier, we can calculate TF start from any BC in a TF. However, we merge TFs into DFs, and therefore you have to select the right BC for the TF start calculation (not just any).

In principle, each unassigned track appears in the AmbiguousTrack table where it is assigned a BC slice or a collision array. One could define the time wrt the first BC in that table, but we have to look up the corresponding association track->ambiguoustrack while the index goes the other way round...

And I realize now that this will not work for unassigned tracks (there the interactionTime is passed as -1), i.e. the tofExpMom will be wrong. In fact, even for assigned tracks the real TOF PID evaluation should use not the nominal interaction time (as passed to fillTrackTablesPerCollision) but precise timeZero.
Now, I am wondering why at all we storing the tofExpMom? It can be always recalculated from precise time-of-flight and stored L-integral.

Hi @shahor02 apologies I missed a bit the discussion because I was traveling.
That said, I agree with you that if the TOF expected momentum is computed as in here i.e.:

              float expBeta = (intLen / tof / cSpeed);
              expMom = mass * expBeta / std::sqrt(1.f - expBeta * expBeta);

writing the expected momentum can be avoided entirely as it can be recomputed on the fly from the quantities in the table.
Nonetheless the TOF expected momentum is to be computed with the expected time of pions and not from the measured TOF.
This also simplifies a bit the issue with the interactionTime as it does not enter in the computation of the expected momentum.
The procedure would be as in https://github.com/alisw/AliPhysics/blob/bb0e58318cd9d7fc90d659a724643b0f2b1c623e/RUN3/AliAnalysisTaskAO2Dconverter.cxx#L1657-L1665 where we use the integrated times for pions to define the expected momentum.

      const AliPID::EParticleType tof_pid = AliPID::kPion;
      const float exp_time = TOFResponse.GetExpectedSignal(track, tof_pid);
      if (exp_time > 0) { // Check that the expected time is positive
        // Expected beta for such hypothesis
        const Float_t exp_beta = (track->GetIntegratedLength() / exp_time / cspeed);

        tracks.fTOFExpMom = AliMathBase::TruncateFloatFraction(
          AliPID::ParticleMass(tof_pid) * exp_beta * cspeed /
            TMath::Sqrt(1. - (exp_beta * exp_beta)),
          mTrack1Pt);
      }

With the present implementation in the RUN3 AOD Produce I don't think that it will fully work as intended in the PID response.

Thanks @njacazio , indeed, the calculation should be (please cross-check)

float expSig = = tofInt.getTOF(o2::track::PID::Pion);
extraInfoHolder.tofSignal = tofMatch.getSignal();  // to be clarified what reference time we use for unassigned tracks
float expMom = 0.f;
if (expSig > 0) {
  float expBeta = (intLen / expSig / cSpeed);
  expMom = mass * expBeta / std::sqrt(1.f - expBeta * expBeta);
}
extraInfoHolder.tofExpMom = expMom;

If you agree, @nburmaso, could you change this.

@jgrosseo concerning your comment:

In principle, each unassigned track appears in the AmbiguousTrack table where it is assigned a BC slice or a collision array.

Could you point on the code you refer to? I am little bit lost, since -1 is provided as a collision ID for unassigned tracks and I don't see where the track times are used at all.

Hi @shahor02 yes, this would work better indeed. Thanks a lot!!

Hi @shahor02
I have adjusted the ambiguous tracks table to what we have discussed in this PR:
#6629
Maybe it makes my point more clear.

@nburmaso : trackRef should be used here instead of trackRefU

Hi @shahor02. Thank you for notice. I have fixed the typo