Adding cluster size to track extra #12044

fmazzasc · 2023-10-09T10:36:33Z

Follows the discussion presented in https://indico.cern.ch/event/1332124/
Cluster sizes are added to AO2Ds and cluster map is removed. Few modifications have been done to the ITS tracking and ITSTPC matching to get the cl size information (pinging @mconcas , @mpuccio , @shahor02 ). We have also updated the AnalysisDataModel and created a versioning of the trackExtra tables. Few dyn columns have been changed, I will undraft the PR as soon as I checked that we get consistent results with the old dataformat (pinging @ddobrigk , @jgrosseo )

shahor02

cannot judge about the backward compatibility of the AOD track model changes, the rest looks ok.

Please consider the following formatting changes to AliceO2Group#12044

jgrosseo

Thanks for this work!

For now, I have only reviewed the changes in AnalysisDataModel. See my minor comments.

However, we have to split this PR in two PRs. The first one introduces the changes in the AnalysisDataModel only, but keeps version 0 as default.
Then when it is merged, you make the PR with the converter in O2Physics
Once that is merged, you make a PR which contains the rest and the change to version 1.

Framework/Core/include/Framework/AnalysisDataModel.h

Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

Framework/Core/src/AODReaderHelpers.cxx

Please consider the following formatting changes to AliceO2Group#12044

fmazzasc · 2023-10-11T10:43:32Z

Thanks to all of you for the comments and the help.

@jgrosseo : I implemented all your comments and switched to 000 as default. As soon as the converter in O2Physics is merged I will move to 001
@aalkin : I changed the data decription as you suggested

I also checked the itsClusterMap with 000 and 001, and the dynamic evaluation works smoothly (see picture). Opening the PR.

mconcas · 2023-10-11T13:30:01Z

ITS side small changes also look ok.

alibuild · 2023-10-14T00:13:23Z

Error while checking build/O2/fullCI for 6282035 at 2023-10-14 02:13:

No log files found

Full log here.

fmazzasc · 2023-10-16T12:37:24Z

Don't you want to factor our the AnalysisDataModel changes in a separate PR and then merge this one once you swap to version 1 and can activate the filling in the AOD producer instead of the commenting out code?

I removed the commented code and opened in draft mode #12087

Framework/Core/include/Framework/AnalysisDataModel.h

Framework/Core/test/test_AnalysisTask.cxx

Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

Framework/Core/include/Framework/AnalysisDataModel.h

fmazzasc · 2023-10-17T09:50:26Z

@shahor02 , @aalkin , could we merge this PR?

Detectors/ITSMFT/ITS/tracking/include/ITStracking/TimeFrame.h

Detectors/GlobalTracking/include/GlobalTracking/MatchTPCITS.h

DataFormats/Detectors/ITSMFT/ITS/include/DataFormatsITS/TrackITS.h

ktf · 2023-10-19T08:01:50Z

DataFormats/Detectors/ITSMFT/common/include/DataFormatsITSMFT/TrkClusRef.h

 struct TrkClusRef : public o2::dataformats::RangeRefComp<4> {
  using o2::dataformats::RangeRefComp<4>::RangeRefComp;
  uint16_t pattern = 0; ///< layers pattern
+  uint32_t clsizes = 0; ///< cluster sizes for each layer


Shouldn't we swap the order of pattern and clsizes to avoid wasting 16 bits in padding per ref?

done, thanks for spotting!

shahor02 · 2023-10-19T08:26:40Z

checks were passed before the trivial swap, merging

* Add ITS cluster size to the AO2D * Add TracksExtra_001 * Update readerhelper * Add cluster sizes to AB * Remove its cluster map * Please consider the following formatting changes * Add table versioning * Fix comments Co-authored-by: Jan Fiete <jgrosseo@cern.ch> * Fix comments (2) Co-authored-by: Jan Fiete <jgrosseo@cern.ch> * Set old TrackExtra as default * Fix data descriptor for trackextra * Please consider the following formatting changes * Fix test * Clean commented code * Fix comment (3) Co-authored-by: Jan Fiete <jgrosseo@cern.ch> * Move dyn columns to versioning namespace * Update Framework/Core/include/Framework/AnalysisDataModel.h * Add braces + unsigned shorts * Move cluster sizes to uint8_t * Swap pattern and clsizes order --------- Co-authored-by: Maximiliano Puccio <maximiliano.puccio@cern.ch> Co-authored-by: ALICE Action Bot <alibuild@cern.ch> Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

mpuccio and others added 5 commits October 6, 2023 09:02

Add ITS cluster size to the AO2D

4e17997

Add TracksExtra_001

32e23d2

Update readerhelper

91def84

Add cluster sizes to AB

37ad278

Remove its cluster map

3480484

fmazzasc requested review from a team, iouribelikov, mconcas, robincaron13 and shahor02 as code owners October 9, 2023 10:36

fmazzasc marked this pull request as draft October 9, 2023 10:36

Please consider the following formatting changes

05b7979

fmazzasc mentioned this pull request Oct 9, 2023

Add Converter for new track extra dataformat AliceO2Group/O2Physics#3572

Merged

shahor02 previously approved these changes Oct 9, 2023

View reviewed changes

Merge pull request #18 from alibuild/alibot-cleanup-12044

1b4d607

Please consider the following formatting changes to AliceO2Group#12044

fmazzasc dismissed shahor02’s stale review via 1b4d607 October 9, 2023 11:56

jgrosseo requested changes Oct 9, 2023

View reviewed changes

fmazzasc and others added 3 commits October 10, 2023 18:02

Add table versioning

c50641a

Fix comments

a5199b5

Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

Fix comments (2)

df1d5ba

Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

aalkin reviewed Oct 11, 2023

View reviewed changes

Framework/Core/src/AODReaderHelpers.cxx Outdated Show resolved Hide resolved

fmazzasc and others added 4 commits October 11, 2023 11:37

Set old TrackExtra as default

ee83f5a

Fix data descriptor for trackextra

1385702

Please consider the following formatting changes

e76dc5e

Merge pull request #19 from alibuild/alibot-cleanup-12044

c3fef7c

Please consider the following formatting changes to AliceO2Group#12044

fmazzasc marked this pull request as ready for review October 11, 2023 10:43

Fix test

6282035

Clean commented code

0e74a5c

fmazzasc mentioned this pull request Oct 16, 2023

Set TrackExtra001 as default #12087

Merged

jgrosseo reviewed Oct 16, 2023

View reviewed changes

Framework/Core/include/Framework/AnalysisDataModel.h Outdated Show resolved Hide resolved

jgrosseo reviewed Oct 16, 2023

View reviewed changes

Framework/Core/include/Framework/AnalysisDataModel.h Outdated Show resolved Hide resolved

jgrosseo reviewed Oct 16, 2023

View reviewed changes

Framework/Core/test/test_AnalysisTask.cxx Show resolved Hide resolved

fmazzasc and others added 3 commits October 16, 2023 15:08

Fix comment (3)

be79370

Co-authored-by: Jan Fiete <jgrosseo@cern.ch>

Move dyn columns to versioning namespace

dfe5933

Merge branch 'dev' into cl_size_ao2d

036a51a

jgrosseo reviewed Oct 16, 2023

View reviewed changes

Framework/Core/include/Framework/AnalysisDataModel.h Outdated Show resolved Hide resolved

Update Framework/Core/include/Framework/AnalysisDataModel.h

db63b19

jgrosseo previously approved these changes Oct 16, 2023

View reviewed changes

shahor02 reviewed Oct 17, 2023

View reviewed changes

Add braces + unsigned shorts

e7f129a

fmazzasc dismissed jgrosseo’s stale review via e7f129a October 17, 2023 15:30

mconcas mentioned this pull request Oct 18, 2023

ITS-Study: create pt resolution, add efficiency of sec study #11763

Merged

Move cluster sizes to uint8_t

cf3b8fa

shahor02 previously approved these changes Oct 19, 2023

View reviewed changes

ktf reviewed Oct 19, 2023

View reviewed changes

Swap pattern and clsizes order

2be5a53

fmazzasc dismissed shahor02’s stale review via 2be5a53 October 19, 2023 08:08

shahor02 merged commit 997cb57 into AliceO2Group:dev Oct 19, 2023

f3sch mentioned this pull request Jun 7, 2024

AOD: Fix aod-thinner for new-ish datamodel changes #13210

Merged

Adding cluster size to track extra #12044

Adding cluster size to track extra #12044

Uh oh!

Conversation

fmazzasc commented Oct 9, 2023

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

jgrosseo left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmazzasc commented Oct 11, 2023

Uh oh!

mconcas commented Oct 11, 2023

Uh oh!

alibuild commented Oct 14, 2023

Uh oh!

fmazzasc commented Oct 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmazzasc commented Oct 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktf Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

fmazzasc Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

shahor02 commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

8 participants

jgrosseo left a comment •

edited

Loading