-
Notifications
You must be signed in to change notification settings - Fork 614
[PWGCF] Add process function for Phi and V0s in filter2Prong #11774
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
[PWGCF] Add process function for Phi and V0s in filter2Prong #11774
Conversation
|
O2 linter results: ❌ 61 errors, |
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.
Thanks a lot. Here some comments!
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.
Great work! A few more comments inline, then we can merge... :)
PWGCF/TableProducer/filter2Prong.cxx
Outdated
|
|
||
| if (grpPhi.DoPhi) { // Process Phi mesons | ||
| for (const auto& cftrack1 : cftracks) { // Loop over first track | ||
| const auto& p1 = tracks.iteratorAt(cftrack1.trackId() - tracks.begin().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.
Why don't we use?
| const auto& p1 = tracks.iteratorAt(cftrack1.trackId() - tracks.begin().globalIndex()); | |
| const auto& p1 = cftrack1.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.
I am not sure this can be done straightforwardly. As far as I understood, cftrack1 only contains the global track and collision indexes, while the physics information are stored in the CFTracks (which however do not contain global index information, but only cf-index ones) and PIDTrack tables. We might need to modify the tables in order to have both the physics and global index information to do it more efficiently than how it is currently done. What do you think?
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 disagree with your statement (Track in CFTrack is an index to the global track to my understanding) but I don't want to delay this PR. We can try it together one of these days.
| /* | ||
| template <typename T> | ||
| using HasProtonPID = decltype(selectionPIDProton(std::declval<T&>())); | ||
| */ |
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.
Shall we remove the commented lines?
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.
Done!
| } | ||
|
|
||
| void processData(soa::Filtered<soa::Join<aod::Collisions, aod::EvSels, aod::CFMultiplicities>>::iterator const& collision, aod::BCsWithTimestamps const&, soa::Filtered<soa::Join<aod::Tracks, aod::TracksExtra, aod::TrackSelection>> const& tracks) | ||
| void processData(soa::Filtered<soa::Join<aod::Collisions, aod::EvSels, aod::CFMultiplicities>>::iterator const& collision, aod::BCsWithTimestamps const&, soa::Filtered<soa::Join<aod::Tracks, aod::TracksExtra, aod::TrackSelection, aod::pidTPCPr, aod::pidTOFPr, aod::TracksDCA>> 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.
If I understand correctly now one cannot use this task anymore without PID information? I think that is not good.
We have to make 2 process functions which then can call a templated one, to avoid the code duplication...
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 split the process function, one for data (default) and one for data pid. They both access a common templated function. Thanks!
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 assume you tested both?
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.
Great! I will now seek feedback from @jaelpark as well.
You can already fix the clang in the meanwhile?
We can then remove the draft status.
PWGCF/TableProducer/filter2Prong.cxx
Outdated
|
|
||
| if (grpPhi.DoPhi) { // Process Phi mesons | ||
| for (const auto& cftrack1 : cftracks) { // Loop over first track | ||
| const auto& p1 = tracks.iteratorAt(cftrack1.trackId() - tracks.begin().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.
I disagree with your statement (Track in CFTrack is an index to the global track to my understanding) but I don't want to delay this PR. We can try it together one of these days.
| } | ||
|
|
||
| void processData(soa::Filtered<soa::Join<aod::Collisions, aod::EvSels, aod::CFMultiplicities>>::iterator const& collision, aod::BCsWithTimestamps const&, soa::Filtered<soa::Join<aod::Tracks, aod::TracksExtra, aod::TrackSelection>> const& tracks) | ||
| void processData(soa::Filtered<soa::Join<aod::Collisions, aod::EvSels, aod::CFMultiplicities>>::iterator const& collision, aod::BCsWithTimestamps const&, soa::Filtered<soa::Join<aod::Tracks, aod::TracksExtra, aod::TrackSelection, aod::pidTPCPr, aod::pidTOFPr, aod::TracksDCA>> 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.
I assume you tested both?
| O2_DEFINE_CONFIGURABLE(cfgCutEta, float, 0.8f, "Eta range for tracks") | ||
|
|
||
| Filter trackFilter = (nabs(aod::track::eta) < cfgCutEta) && (aod::track::pt > cfgCutPt); | ||
| Filter trackSelection = (requireGlobalTrackInFilter()) || (aod::track::isGlobalTrackSDD == (uint8_t) true); |
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.
There's a problem with the clang-format and the subsequent checks, they are not compatible. To pass the test, this needs to be manually changed back to (uint8_t) true (with the space) each time before commit (same for the other one higher up).
|
Hi @jaelpark ! Thanks a lot for your suggestions, I implemented them in the last commit. Let me know if you have further comments |
Please consider the following formatting changes to AliceO2Group#11774
|
Thanks! No other comments from me. |
This PR adds the possibility to process Phi and V0s in the filter2Prong code. PID selections can be applied at the filterCorrelation and filter2Prong levels. @skundu692 @jgrosseo @prottayCMT for your information.