-
Notifications
You must be signed in to change notification settings - Fork 613
[PWGJE] Add photon-hadron correlation analysis task #11433
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
Conversation
|
O2 linter results: ❌ 1 errors, |
|
The failed O2 linter check seems to stem only from other workflows in |
nzardosh
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.
Thanks for the work and welcome to analysis on O2 :) I have left a very short guiding review, however you can wait for a more detailed review from the EMCal side by @fjonasALICE . For now the main issues I spot is the way the JE derived tables are interfaced with the non-JE tables and also that it seems in more places JE tables could be used. Thanks
| PROCESS_SWITCH(CorrelationTableProducer, processRecoCollisionTrigger, "process correlation collision_extra and trigger table (reconstructed)", false); | ||
|
|
||
| void processRecoPipmTPCTOF(aod::JetCollision const& collision, | ||
| soa::Join<aod::JetTracks, aod::TracksExtra, aod::TrackSelection, aod::pidTPCPi, aod::pidTOFPi> const& tracks) |
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.
The JetTracks table is not joinable by default to the other Track tables. You need to link them through the JTracksPI table as shown in the jettutorial task
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 have adjusted the task in the commit "PR_adjustments"
| // hadron/pipm | ||
| for (auto const& track : tracks) { | ||
| // track selection | ||
| if (!checkGlobalTrackEta(track)) |
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.
you can use the track selections implemented in the JE framework
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.
Adjusted in the commit "PR_adjustments"
| return; | ||
|
|
||
| // photonsPCM (for some reason collsionId not an index column (?)) | ||
| auto const v0PhotonsThisEvent = v0Photons.sliceBy(perColV0Photons, collision.collision().globalIndex()); |
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.
instead of collision.collision().globalIndex() you can do collision.collisionId()
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.
Adjusted in the commit "PR_adjustments"
| auto const v0PhotonsThisEvent = v0Photons.sliceBy(perColV0Photons, collision.collision().globalIndex()); | ||
|
|
||
| // photonPCM | ||
| for (auto const& v0Photon : v0PhotonsThisEvent) { |
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.
where are the v0Photons produced?I guess these are objects from LF. @fjonasALICE do we expect to use these or do we have our own photon/JCluster tables that can be used here instead?
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.
these are not emcal and i am no expert for the V0 in O2
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.
Reply in the original comment below
| } | ||
| PROCESS_SWITCH(PhotonChargedTriggerCorrelation, processCorrFirst, "process to gather info before correlation processes", false); | ||
|
|
||
| void processCorrHadron(CorrCollision const& collision, aod::Triggers const& triggers, aod::Hadrons const& hadrons) |
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.
Is this trying to use triggered data? If so please use the JE methods and tables for this
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.
As explained in the original comment, the task is not built for running on triggered data, so I would not know what tables to include. The only software-trigger-related part of the code is an addition in the event selection (used for a test and disabled by default) via "triggerMaskBits" (as in the jetTutorial task).
|
Hello, thank you for the quick response. I have adjusted the task. |
|
@nzardosh @fjonasALICE |
fjonasALICE
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
@nzardosh since no emcal is used I can't comment a lot on the V0. I went through the code for a few general comments. For detailled review the number of changes is too large, but if the code is tested, and given it does not touch anything else, I think it is fine to approve once the requested changes are in. I also did not see if @jkinner2 implemented the changes for the JE software triggers, and one should double check that the V0 candidates can be properly matched to the index of JCollisions
| auto const v0PhotonsThisEvent = v0Photons.sliceBy(perColV0Photons, collision.collision().globalIndex()); | ||
|
|
||
| // photonPCM | ||
| for (auto const& v0Photon : v0PhotonsThisEvent) { |
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.
these are not emcal and i am no expert for the V0 in O2
| /// Also contains checks and monte-carlo (efficiency, purity, mc-true correlation,...) | ||
| /// End goal of studying correlations between direct photons and jets | ||
|
|
||
| #define ETA_MAX_DEFAULT 0.8 |
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.
do we really need to define like this? I think a const double etc in the current namespace would be the better idea
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 have implemented it in the commit "change_define_to_variable__move_table_definitions_to_own_header"
| // derived data for correlations (on-the-fly) | ||
| // should be more efficient due to usage in correlations and event mixing | ||
|
|
||
| namespace o2::aod |
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 would propose to move table definitions to their own header file, like it is done for other analysis task
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 have adjusted the task in the commit "change_define_to_variable__move_table_definitions_to_own_header"
|
@nzardosh @fjonasALICE |
Adding task for analysing angular photon-jet correlations in data (PWG-JE JP PAG presentation: https://indico.cern.ch/event/1546154/#22-photon-hadron-correlations)