[EMCAL-732] implementation of EMCAL standalone AOD producer workflow by fjonasALICE · Pull Request #7644 · AliceO2Group/AliceO2

fjonasALICE · 2021-11-17T16:45:43Z

No description provided.

mfasDa

Only a small change from my side requested

mfasDa · 2021-11-17T16:55:43Z

Detectors/EMCAL/workflow/include/EMCALWorkflow/StandaloneAODProducerSpec.h

+  static const char* BININGCELLS;
+  static const char* BINDINGCELLSTRG;


No longer needed.

mfasDa

The BC mapping does not work, when testing I get negative values in the index column and consequently a crash once I try to access the BC. This must be changed using a counter for the BC ID that is used for all 4 tables. The BC table in this case can be filled inside the event loop.

mfasDa · 2021-11-17T19:36:53Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+
+      // fill table
+      caloCellsCursor(0,
+                      bcID,


This is not going to work. We need the index in the BC table, not the global BCID. Since the BCs are sorted in the trigger record table it is sufficient to add counter and fill the BC table for the same event right after the cell and calo table with the index the same counter value.

mfasDa · 2021-11-17T19:37:15Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+
+    // filled only with BCID, rest dummy for no2
+    caloCellsTRGTableCursor(0,
+                            bcID,


Needs same BC counter as mentioned before.

mfasDa · 2021-11-17T19:37:23Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+
+    // fill collision cursor
+    collisionsCursor(0,
+                  bcID,


mfasDa · 2021-11-17T19:38:23Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+  std::sort(bcMap.begin(), bcMap.end(), std::less<int64_t>());
+  uint64_t triggerMask = 1;
+  for (auto& bcID : bcMap) {
+    bcCursor(0,
+             runNumber,
+             bcID,
+             triggerMask);
+  }


This should go directly in the event loop. Instead of filling 0 in the index column we use the counter value used in the other tables.

jgrosseo · 2021-11-18T15:46:15Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+
+    LOG(INFO) << "Found " << cellsInEvent.size() << " cells in event";
+
+    auto bcID = interactionRecord.toLong();


This is wrong. You have to fill into the tables the tree entry of the BC entry in the BC tree. You get this with bcCursor.lastIndex() after filling the respective BC entry.
So you need a loop over BCs, you cannot fill first one, then the other (unless you know which ones you fill and you do not reorder as you do below)

bcCursor is a lambda function, lastIndex() doesn't exist. Can't we just take a running index (i.e. iev) that we increment per event/bc in the loop?

- Using event loop index for BC Index - Fill BC table in loop over events

mfasDa

Tested and working, should be fine now. @jgrosseo Could you please merge once the CI finishes?

shahor02 · 2021-11-18T19:38:28Z

sorry I've missed something, but what is the point of the standalone producer? Isn't it better to change the usual one is such a way that it supports EMCal only?

mfasDa · 2021-11-18T20:28:00Z

@shahor02 I was actually checking with you in a private Mattermost conversation whether there is any chance to have the AODProducer supporting also stand-alone runs. Currently the global producer will not work if there is not at least FIT and a primary vertex reconstructed, which we don't have in the stand-alone run. Definitely we want AODs from the stand-alone runs we took in the pilot beam, we would like to test the analysis chain using the analysis framework, we want to repeat the pi0 analysis using the run3 analysis framework as validation of the analysis task. As you see the conversion from CTF to AOD for triggered detectors in the stand alone case is super simple, because we just need to port the cell container and link with the BC table, where the BC we provide ourselves. Adding this option to the global AODProducer will need quite sizeable modification. I am not generally against adding a stand alone mode to the global AOD producer, I also prefer common code instead of many code duplications, and maybe other detectors have similar interests as well. But for such a simple purpose I am wondering whether a stand-alone workflow is the more efficient way to go.

shahor02 · 2021-11-18T20:44:24Z

I've just rechecked our mm conversation, there I was also suggested to make the standard aodproducer to work with caloriemeters only. it should not be a big deal, I think one should simply foresee the possibility to run w/o primary vertex and use triggered detectors triggers to create globalBC maps.
This standalone code is simple, but if the something is changing in the AOD, you will need to change it also here...
Anyway, one can have this for the moment, but I would suggest eventually switching to the standard one. Pinging to @nburmaso .

mfasDa · 2021-11-19T08:51:38Z

@shahor02 Thanks for your comment! If the option would be foreseen in the standard AOD producer that would of course be better. Still it does not hurt to merge this one, because then we have something in the mean time to work with and commission the EMCAL analysis framework. Once the global producer is ready to support stand-alone runs of triggered detectors we will move to that and delete this one again.

shahor02 · 2021-11-19T08:57:39Z

Detectors/EMCAL/workflow/src/StandaloneAODProducerSpec.cxx

+  uint64_t tfNumber;
+  if (mTFNumber == -1L) {
+    // TODO has to be made globally unique (by using absolute time of TF). For now is unique within the run
+    tfNumber = dh->tfCounter; // getTFNumber(startIR, runNumber);


Note that this part was changed in the standard AOD producer:

AliceO2/Detectors/AOD/src/AODProducerWorkflowSpec.cxx

Line 1101 in c394a04

tfNumber = uint64_t(dh->firstTForbit) + (uint64_t(dh->runNumber) << 32); // getTFNumber(mStartIR, runNumber);

I'll fix this and merge

Thanks a lot for fixing!

shahor02

all tests were passed, merging after applying a trivial fix

mfasDa · 2021-11-19T09:10:31Z

@shahor02 Thanks a lot for merging! We promise we will migrate to the standard AOD producer once it supports stand alone runs and remove this one again. We will however not mess around with changing other tables there, this should be done by the experts, too much damage could be done for other detectors if something goes wrong in the implementation.

fjonasALICE requested a review from mfasDa as a code owner November 17, 2021 16:45

mfasDa requested changes Nov 17, 2021

View reviewed changes

fjonasALICE added 3 commits November 18, 2021 13:18

[EMCAL-732] standalone AOD producer workflow for EMC

80daf9b

[EMCAL-732] adapted cmake list for standalone AO2D producer

8ff2446

[EMCAL-732] added collision table to EMC StandaloneAODProducerr

b230a92

jgrosseo reviewed Nov 18, 2021

View reviewed changes

Fix index handling of BC ID

dfb4870

- Using event loop index for BC Index - Fill BC table in loop over events

fjonasALICE force-pushed the EMCAL-732 branch from 8fc1a75 to dfb4870 Compare November 18, 2021 18:37

mfasDa self-requested a review November 18, 2021 18:39

mfasDa previously approved these changes Nov 18, 2021

View reviewed changes

shahor02 reviewed Nov 19, 2021

View reviewed changes

fix tfNumber assignment

f25d6ae

shahor02 dismissed mfasDa’s stale review via f25d6ae November 19, 2021 08:58

shahor02 approved these changes Nov 19, 2021

View reviewed changes

shahor02 merged commit b31ae36 into AliceO2Group:dev Nov 19, 2021

		static const char* BININGCELLS;
		static const char* BINDINGCELLSTRG;


		LOG(INFO) << "Found " << cellsInEvent.size() << " cells in event";

		auto bcID = interactionRecord.toLong();

Conversation

fjonasALICE commented Nov 17, 2021

Uh oh!

mfasDa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfasDa 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!

jgrosseo Nov 18, 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!

mfasDa left a comment

Choose a reason for hiding this comment

Uh oh!

shahor02 commented Nov 18, 2021

Uh oh!

mfasDa commented Nov 18, 2021

Uh oh!

shahor02 commented Nov 18, 2021

Uh oh!

mfasDa commented Nov 19, 2021

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!

mfasDa commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

jgrosseo Nov 18, 2021 •

edited

Loading