Skip to content

BC selection task and alias accounting added#5757

Merged
iarsene merged 4 commits into
AliceO2Group:devfrom
ekryshen:dev
Mar 25, 2021
Merged

BC selection task and alias accounting added#5757
iarsene merged 4 commits into
AliceO2Group:devfrom
ekryshen:dev

Conversation

@ekryshen
Copy link
Copy Markdown
Contributor

Switching to bc-based selection:

  • All trigger and forward detector timing decisions are filled in a dedicated BcSels table joinable with BCs table.
  • Added Run2MatchedToBCSparse in AODReaderHelpers to link bcs with fit and zdc detectors.
  • Keeping EvSels table (joinable with Collisions table) for the moment. Filled directly from BcSels table.
  • Added bc-based alias accounting

@ekryshen ekryshen requested review from a team, iarsene and jgrosseo as code owners March 20, 2021 07:11
@jgrosseo
Copy link
Copy Markdown
Collaborator

jgrosseo commented Mar 22, 2021

Does everyone need always both tasks (event selection and bc selection), or shall we make that configurable / in split files?

@ekryshen
Copy link
Copy Markdown
Contributor Author

Does everyone need always both tasks (event selection and bc selection), or shall we make that configurable / in split files?

Most of our analyses rely on collision-based event selection so they will always need the bc-based selection in the pipe. On the other hand, bc-based analyses (like muon UPC) do not need collision-based event selection. I am not sure if we need excepions for this rare case. I prefer to avoid configurables now because event-selection qa and multiplicity tasks rely on the presence of the evsel table.

@jgrosseo
Copy link
Copy Markdown
Collaborator

jgrosseo commented Mar 22, 2021

Thanks for your reply. I agree. Let me test it also with my tasks, before we merge.

@jgrosseo
Copy link
Copy Markdown
Collaborator

Hi @ekryshen
This runs well for me. I see a few more collision selected with the new code than with the old one. Those collisions have different multiplicities (also > 1000 tracks). Is this change expected?
I also post a few comments to the code

{
// TODO bool arrays are not supported? Storing in int32 for the moment
DECLARE_SOA_COLUMN(Alias, alias, int32_t[kNaliases]);
DECLARE_SOA_COLUMN(BBT0A, bbT0A, bool); // beam-beam time in T0A
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to define the columns again. I propose you put them in the same name space, and then you can reuse them for the second definition

@ekryshen
Copy link
Copy Markdown
Contributor Author

ekryshen commented Mar 24, 2021

This runs well for me. I see a few more collision selected with the new code than with the old one. Those collisions have different multiplicities (also > 1000 tracks). Is this change expected?

Thanks for the confirmation. I suspect those extra events are due to a bug in trigger aliases stored in CCDB (essentially counting some events from triggerMaskNext50). I found and fixed it in the morning. If you ran your code yesterday, that can explain extra counts.

I will check other comments to the code now.

@jgrosseo
Copy link
Copy Markdown
Collaborator

jgrosseo commented Mar 24, 2021

I found and fixed it in the morning. If you ran your code yesterday, that can explain extra counts.

I ran my code this morning (few minutes before my comment), both: with and without your changes, so it should have picked up the same CCDB

kNaliases
};

static const std::string aliasLabels[kNaliases] = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't const char* enough?

Copy link
Copy Markdown
Collaborator

@jgrosseo jgrosseo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still had a comment but I am fine with merging this, you can change that eventually in the next PR

@ekryshen
Copy link
Copy Markdown
Contributor Author

Hi @jgrosseo, ok, thanks. Just for the record: the difference in the old and new event selection was traced back to the issue in the Run2MatchedSparse matcher that was used in the old approach. Anton is looking into it.

@iarsene iarsene merged commit d9fa993 into AliceO2Group:dev Mar 25, 2021
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Nov 22, 2021
* BC selection task and alias accounting added

* clang-format fixes

* bcsel namespace removed. alias labels added. +bug fixes

* clang-format fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants