Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Add ZDC table to the AOD producer#7343

Merged
sawenzel merged 14 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
nburmaso:aod-updatesnburmaso/AliceO2:aod-updatesCopy head branch name to clipboard
Dec 6, 2021
Merged

Add ZDC table to the AOD producer#7343
sawenzel merged 14 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
nburmaso:aod-updatesnburmaso/AliceO2:aod-updatesCopy head branch name to clipboard

Conversation

@nburmaso
Copy link
Contributor

@nburmaso nburmaso commented Oct 18, 2021

Hi @sawenzel, @shahor02. While the reader problem is not resolved yet (related to some issue in the reader, which I contacted Ruben about; see log attached), I'm putting the code I prepared for the ZDC table in this draft PR to have it ready.

test.log

@nburmaso nburmaso requested a review from a team as a code owner October 18, 2021 08:56
@nburmaso nburmaso marked this pull request as draft October 18, 2021 09:03

// zdc helper maps to avoid a number of "if" statements
map<string, float> mZDCEnergyMap;
map<int, float> mZDCTDCMap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add what is being mapped to what in the comments?
Is map better than unordered_map ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sawenzel. I have added some comments for these maps. Concerning performance of map and unordered_map, I think in this case there should be no real differences, since these maps will consist of a small number of key-value pairs. Here, keys correspond to ZDC sub-parts and TDC channels, so their number is small indeed. Perhaps, performance-wise it would be better to simply use if-clauses, but there will be a lot of them :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello,
I'm from the ZDC team. To me it looks a bit strange to have a map that uses signal TDC ids and another that uses signal names. One should probably unify the approach and use signal names for both maps. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cortesep. Thank you for your comment. I will try to clean it up a bit.

O2::SimConfig
O2::DataFormatsFT0
O2::Steer
O2::ZDCBase
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this global change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it is a typo. I just added ZDCBase, but accidentally indented some lines.

@sawenzel sawenzel marked this pull request as ready for review November 1, 2021 07:53
sawenzel
sawenzel previously approved these changes Nov 19, 2021
@sawenzel sawenzel enabled auto-merge (squash) November 19, 2021 12:50
@sawenzel sawenzel disabled auto-merge November 22, 2021 13:22
@sawenzel sawenzel enabled auto-merge (squash) November 22, 2021 13:22
@sawenzel
Copy link
Collaborator

@nburmaso : I thought this was merged already but apparently the auto-merge feature didn't go through. Unfortunately, we now have merge conflicts again. Could you please take a look?

auto-merge was automatically disabled November 29, 2021 13:28

Head branch was pushed to by a user without write access

@nburmaso
Copy link
Contributor Author

Hi @sawenzel. I have resolved the conflicts and cleaned up the code a bit.

nburmaso added a commit to nburmaso/AliceO2 that referenced this pull request Dec 1, 2021
@sawenzel sawenzel enabled auto-merge (rebase) December 6, 2021 12:06
@sawenzel sawenzel disabled auto-merge December 6, 2021 12:06
@sawenzel sawenzel enabled auto-merge (squash) December 6, 2021 12:07
@sawenzel sawenzel merged commit 28d279d into AliceO2Group:dev Dec 6, 2021
hhillema pushed a commit to ezradlesser/AliceO2 that referenced this pull request Dec 9, 2021
@nburmaso nburmaso deleted the aod-updates branch February 24, 2022 08:39
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.