-
Notifications
You must be signed in to change notification settings - Fork 483
Add PMD info to AO2Ds #13998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PMD info to AO2Ds #13998
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Hello @ddobrigk, could you please have a look at my modifications? |
Please consider the following formatting changes to AliceO2Group#13998
|
This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days. |
|
Hello @ddobrigk, good morning! |
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Error while checking build/O2/fullCI_slc9 for f75ba12 at 2025-04-11 02:39: Full log here. |
|
Dear @ddobrigk, @pzhristov, @ktf, and @jgrosseo, I am not sure if the error is due to adding the PMD info. Could you please have a look? Thanks, |
jgrosseo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
The code itself is fine with me. Could you move it into the section starting with
// ---- Run 2 tables ----
and into the run 2 name space (only columns, not the table definition)?
Many thanks!
Changes are done based on suggestions from Jan Fiete
Please consider the following formatting changes to AliceO2Group#13998
|
Dear @jgrosseo, Thanks, |
|
Thanks! Something is weird because your new table is gone? You somehow added it to an existing one. Maybe there was a misunderstanding you still need an own table. Just it should have been moved next to the other run 2 tables (the column definitions are already in the right place...) |
|
Your initial commit was fine, your lines should have just been moved into a different part of the file, so that it is logically more coherent. And as it is part of run 2 it makes very much sense to have it in the run 2 name space. What do you think? Also, please run a test of your code on a converted AO2D.root which contains the PMD information and try to fill a simple histogram. This makes sure that the code works fine. |
|
Dear @jgrosseo, DECLARE_SOA_TABLE(Pmds, "AOD", "PMD", //! Photon information from PMD detector Please let me know if it is okay to you. Thanks, |
Please consider the following formatting changes to AliceO2Group#13998
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
jgrosseo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing that you add your columns to 2 different tables, why so?
(See also Mattermost discussion)
jgrosseo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for CI (first time committer). Commits still to be squashed.
|
Dear @jgrosseo, Thanks, |
|
Dear @jgrosseo, Thanks, |
I have added the PMD info to AO2Ds.
The conversion of PMD data is already discussed by David in his presentations at ALICE Physics Week and WP4 + WP14 meeting.