Add more information to TrackExtra table #6596

nburmaso · 2021-07-06T13:24:21Z

No description provided.

shahor02

Hi @nburmaso
Please see comments below. BTW, on Monday we also discussed the s.vertices storage, we may eventually store not only prong indices but also corresponding track.getX() params. But at the moment, could you simply add the indices only, just to have the V0s stored?

shahor02 · 2021-07-06T18:52:42Z

Detectors/AOD/include/AODProducerWorkflow/AODProducerWorkflowSpec.h

 using namespace o2::framework;
 using GID = o2::dataformats::GlobalTrackID;
 using DataRequest = o2::globaltracking::DataRequest;
+using TPCClRefElem = uint32_t;


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

Ok, I will clean it up

shahor02 · 2021-07-06T19:25:58Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

      extraInfoHolder.tofExpMom = -999.f;
      extraInfoHolder.trackEtaEMCAL = -999.f;
      extraInfoHolder.trackPhiEMCAL = -999.f;
      auto& trackIndex = primVerGIs[ti];


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

shahor02 · 2021-07-06T19:35:20Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

      extraInfoHolder.trackEtaEMCAL = -999.f;
      extraInfoHolder.trackPhiEMCAL = -999.f;
      auto& trackIndex = primVerGIs[ti];
      if (src == GIndex::Source::ITS && mFillTracksITS) {


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

AliceO2/Detectors/GlobalTrackingWorkflow/src/primary-vertexing-workflow.cxx

Line 61 in 3d7bf78

GID::mask_t allowedSourcesVT = GID::getSourcesMask("ITS,MFT,TPC,ITS-TPC,,TPC-TOF,TPC-TRD,ITS-TPC-TRD,ITS-TPC-TOF");

AliceO2/Detectors/GlobalTrackingWorkflow/src/primary-vertexing-workflow.cxx

Line 74 in 3d7bf78

GID::mask_t srcVT = allowedSourcesVT & GID::getSourcesMask(configcontext.options().get<std::string>("vetex-track-matching-sources"));

?
Then you can simply check
if (GIndex::includesSource(src, sourcesMask)) {

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

shahor02 · 2021-07-06T19:40:35Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

        addToTracksTable(tracksCursor, tracksCovCursor, track, -1, src);
        addToTracksExtraTable(tracksExtraCursor, extraInfoHolder);
      }
      if (src == GIndex::Source::TPC && mFillTracksTPC) {


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.

shahor02 · 2021-07-06T19:44:48Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

+        addToTracksExtraTable(tracksExtraCursor, extraInfoHolder);
+      }
+      if (src == GIndex::Source::TPCTRD && mFillTracksTPC) {
+        const auto& track = tracksTPCTRD[trackIndex.getIndex()];


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

AliceO2/Framework/Core/include/Framework/AnalysisDataModel.h

Lines 218 to 221 in dc091db

DECLARE_SOA_DYNAMIC_COLUMN(TPCNClsFound, tpcNClsFound, //! Number of found TPC clusters

[](uint8_t tpcNClsFindable, int8_t tpcNClsFindableMinusFound) -> int16_t { return (int16_t)tpcNClsFindable - tpcNClsFindableMinusFound; });

DECLARE_SOA_DYNAMIC_COLUMN(TPCNClsCrossedRows, tpcNClsCrossedRows, //! Number of crossed TPC Rows

[](uint8_t tpcNClsFindable, int8_t TPCNClsFindableMinusCrossedRows) -> int16_t { return (int16_t)tpcNClsFindable - TPCNClsFindableMinusCrossedRows; });

, so they can't be used in a file.

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.

shahor02 · 2021-07-06T19:56:37Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

        extraInfoHolder.tofChi2 = tofMatch.getChi2();
        const auto& tofInt = tofMatch.getLTIntegralOut();
        extraInfoHolder.tofSignal = tofInt.getTOF(0); // fixme: what id should be used here?
        extraInfoHolder.length = tofInt.getL();


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

AliceO2/Analysis/DataModel/include/AnalysisDataModel/PID/PIDTOF.h

Lines 58 to 61 in 3d7bf78

static float GetExpectedSignal(const float& mom, const float& mass);

/// Gets the expected beta given the particle index (no energy loss taken into account)

float GetExpectedSignal(const Coll& col, const Trck& trk) const { return GetExpectedSignal(trk.p(), o2::track::PID::getMass2Z(id)); }

but I am not sure about the template parameters, hope @njacazio can clarify what should be used

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

nburmaso · 2021-07-07T13:19:09Z

Hi @shahor02, @sawenzel. I have tried addressing the comments and updated the code. I have moved track table filling into a separate subroutine and also removed track flags, such as --fill-tracks-its, so information sources can now be passed via --info-sources flag as a comma-separated list. I still have some doubts about TPC clusters. Is this implementation

AliceO2/Detectors/AOD/src/AODProducerWorkflowSpec.cxx

Lines 236 to 240 in 2a9f3f9

    
           uint8_t shared, found, crossed; // fixme: need to switch from these placeholders to something more reasonable 
        
           countTPCClusters(tpcOrig, tpcClusRefs, tpcClusShMap, tpcClusAcc, shared, found, crossed); 
        
           extraInfoHolder.tpcNClsFindable = tpcOrig.getNClusters(); 
        
           extraInfoHolder.tpcNClsFindableMinusCrossedRows = tpcOrig.getNClusters() - crossed; 
        
           extraInfoHolder.tpcNClsShared = shared;

is good for now as a placeholder?

shahor02 · 2021-07-07T13:33:56Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

+            countTPCClusters(tpcOrig, tpcClusRefs, tpcClusShMap, tpcClusAcc, shared, found, crossed);
+            extraInfoHolder.tpcNClsFindable = tpcOrig.getNClusters();
+            extraInfoHolder.tpcNClsFindableMinusCrossedRows = tpcOrig.getNClusters() - crossed;
+            extraInfoHolder.tpcNClsShared = shared;


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

sawenzel · 2021-07-07T13:48:44Z

Took away the "draft" label to trigger CI. So with this PR we have most of TPC and TOF data filled?

nburmaso · 2021-07-07T14:15:40Z

@sawenzel: Yes, it seems that we have most of the columns covered with this PR

shahor02 · 2021-07-07T14:16:20Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

+            float mom = trackPar.getP();
+            float mass = o2::constants::physics::MassPionCharged; // default pid = pion
+            float expSig = mom > 0 ? std::sqrt(mom * mom + mass * mass) : 0.f;
+            extraInfoHolder.tofSignal = expSig;
+            float expMom = 0.f;
+            if (expSig > 0) {
+              float expBeta = (intLen / expSig / cSpeed);
+              expMom = mass * expBeta * cSpeed / std::sqrt(1.f - (expBeta * expBeta));
+            }
+            extraInfoHolder.tofExpMom = expMom;


This does not look correct:

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?

add TOFsignal in TOF matched info

shahor02

I think there is some general inconsistency in the TOF info stored, see comments below.
Pinging to @pzhristov @jgrosseo @njacazio

shahor02 · 2021-07-08T10:15:56Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

+            extraInfoHolder.length = intLen;
+            float mass = o2::constants::physics::MassPionCharged; // default pid = pion
+            float expSig = tofMatch.getSignal();
+            extraInfoHolder.tofSignal = expSig;


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

AliceO2/Detectors/AOD/src/AODProducerWorkflowSpec.cxx

Line 175 in af43e33

truncateFloatFraction(extraInfoHolder.tofSignal, mTrackSignal),

directly, w/o truncation.
Then, the AliAnalysisTaskAO2Dconverter should be fixed in AliPhysics, and the lines

AliceO2/Framework/Core/include/Framework/AnalysisDataModel.h

Line 209 in 2360d52

DECLARE_SOA_COLUMN(TOFSignal, tofSignal, float); //! TOF signal matched to the track

and

AliceO2/Framework/Core/include/Framework/AnalysisDataModel.h

Lines 214 to 215 in 2360d52

DECLARE_SOA_DYNAMIC_COLUMN(HasTOF, hasTOF, //! Flag to check if track has a TOF measurement

[](float tofSignal, float tofExpMom) -> bool { return (tofSignal > 0.f) && (tofExpMom > 0.f); });

should be redefined with double tofSignal.
Pinging @pzhristov and @jgrosseo

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):

AliceO2/Detectors/AOD/src/AODProducerWorkflowSpec.cxx

Lines 762 to 767 in af43e33

auto& timeStamp = vertex.getTimeStamp();

double tsTimeStamp = timeStamp.getTimeStamp() * 1E3; // mus to ns

uint64_t globalBC = std::round(tsTimeStamp / o2::constants::lhc::LHCBunchSpacingNS);

LOG(DEBUG) << globalBC << " " << tsTimeStamp;

// collision timestamp in ns wrt the beginning of collision BC

tsTimeStamp = globalBC * o2::constants::lhc::LHCBunchSpacingNS - tsTimeStamp;

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...

shahor02 · 2021-07-08T10:46:03Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

+            extraInfoHolder.tofSignal = expSig;
+            float expMom = 0.f;
+            if (expSig > 0) {
+              float tof = expSig - interactionTime;


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.

sawenzel · 2021-07-08T12:44:54Z

It looks like some fundamental questions remain. Can we still merge this since it's in any case an improvement? And then TOF can take directly take in adjusting it's information?

shahor02 · 2021-07-08T12:50:04Z

@sawenzel fine for me

shahor02 · 2021-07-09T11:20:56Z

Detectors/AOD/src/AODProducerWorkflowSpec.cxx

-    }
-  }
+  // fixme: interaction time is undefined for unassigned tracks (?)
+  fillTrackTablesPerCollision(-1, -1, trackRefU, primVerGIs, recoData, tracksCursor, tracksCovCursor, tracksExtraCursor, mftTracksCursor);


@nburmaso : trackRef should be used here instead of trackRefU

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

sawenzel · 2021-07-12T10:50:02Z

Merging now as is (future improvements coming in other PRs). Tested it locally.

Add more information to TrackExtra table

b0dce92

shahor02 reviewed Jul 6, 2021

View reviewed changes

Reorganize code to reduce repeating parts. Add tofExpMom to TrackExtra.

2a9f3f9

shahor02 reviewed Jul 7, 2021

View reviewed changes

sawenzel marked this pull request as ready for review July 7, 2021 13:47

sawenzel requested a review from a team as a code owner July 7, 2021 13:47

Add tpcNClsFindableMinusFound

3dd8fed

shahor02 reviewed Jul 7, 2021

View reviewed changes

noferini and others added 3 commits July 7, 2021 17:48

add TOFsignal in TOF matched info

a5be3a3

Merge pull request #1 from noferini/aod-updates

09156f1

add TOFsignal in TOF matched info

Fix tofExpMom calculation

af43e33

shahor02 reviewed Jul 8, 2021

View reviewed changes

shahor02 reviewed Jul 9, 2021

View reviewed changes

nburmaso added 2 commits July 9, 2021 14:33

Fix typos in fillTrackTablesPerCollision() calls

ca8bb0d

Get expSig via getTOF()

8d9492b

sawenzel merged commit 2fc1634 into AliceO2Group:dev Jul 12, 2021

nburmaso deleted the aod-updates branch July 14, 2021 06:55

	DECLARE_SOA_DYNAMIC_COLUMN(TPCNClsFound, tpcNClsFound, //! Number of found TPC clusters
	[](uint8_t tpcNClsFindable, int8_t tpcNClsFindableMinusFound) -> int16_t { return (int16_t)tpcNClsFindable - tpcNClsFindableMinusFound; });
	DECLARE_SOA_DYNAMIC_COLUMN(TPCNClsCrossedRows, tpcNClsCrossedRows, //! Number of crossed TPC Rows
	[](uint8_t tpcNClsFindable, int8_t TPCNClsFindableMinusCrossedRows) -> int16_t { return (int16_t)tpcNClsFindable - TPCNClsFindableMinusCrossedRows; });

	static float GetExpectedSignal(const float& mom, const float& mass);

	/// Gets the expected beta given the particle index (no energy loss taken into account)
	float GetExpectedSignal(const Coll& col, const Trck& trk) const { return GetExpectedSignal(trk.p(), o2::track::PID::getMass2Z(id)); }

	DECLARE_SOA_DYNAMIC_COLUMN(HasTOF, hasTOF, //! Flag to check if track has a TOF measurement
	[](float tofSignal, float tofExpMom) -> bool { return (tofSignal > 0.f) && (tofExpMom > 0.f); });

	auto& timeStamp = vertex.getTimeStamp();
	double tsTimeStamp = timeStamp.getTimeStamp() * 1E3; // mus to ns
	uint64_t globalBC = std::round(tsTimeStamp / o2::constants::lhc::LHCBunchSpacingNS);
	LOG(DEBUG) << globalBC << " " << tsTimeStamp;
	// collision timestamp in ns wrt the beginning of collision BC
	tsTimeStamp = globalBC * o2::constants::lhc::LHCBunchSpacingNS - tsTimeStamp;

Add more information to TrackExtra table #6596

Add more information to TrackExtra table #6596

Uh oh!

Conversation

nburmaso commented Jul 6, 2021

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nburmaso Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nburmaso commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sawenzel commented Jul 7, 2021

Uh oh!

nburmaso commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrosseo Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njacazio Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nburmaso Jul 7, 2021 •

edited

Loading

jgrosseo Jul 8, 2021 •

edited

Loading

njacazio Jul 9, 2021 •

edited

Loading

nburmaso Jul 9, 2021 •

edited

Loading