Add pt-dependent tracking efficiency method

[jaimenorman]

[alibuild]

53ec48d4d9cacd892db16f15de90d5df868815ea: approval required: 1 of @adriansev (Adrian Sevcenco), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @germainma (Marie Germain), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @lcunquei (Leticia Cunqueiro Mendez), @loizides (Constantinos Loizides), @matplo (Mateusz Ploskon), @mfasDa (Markus Fasel), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @raymondEhlers (Raymond James Ehlers), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @vkucera (Vit Kucera)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

@mverwe @raymondEhlers I was hoping one of you could take a look at this draft pull request to implement a pT-dependent efficiency ( also feel free to copy in others if there is anyone else you think could help review).

The efficiency is taken from a yaml file from an array where the efficiency is defined in pt bins. In the yaml file there is an array containing the pT-bin limits as well as the efficiencies.

The pt dependent efficiencies can be set in the task configuration

task->SetTrackingEfficiencyFromYAML("LHC18q_010", "/bundle/data/ALICE/alice/AliPhysics/PWGJE/EMCALJetTasks/TrackEfficiencyConfiguration.yaml");

the first option sets the period and centrality (in the form "period_centrality") and the second option sets the yaml file path (which is set by default to the path which will contain this file) . There is then a function in the task which defines a histogram based on the information taken from the yaml file. The efficiencies in the yaml array correspond to the relative uncertainty in each bin. One thing to note is that the function SetTrackEfficiency will still work and will add an extra flat efficiency, which will be needed for e.g. embedding, where the nominal PYTHIA track efficiency is reduced by 2%

I think perhaps the period could be set in a more automatic way, but otherwise I have tested on some pp events and things work as expected. Any feedback would be helpful anyway!

[mverwe]

Hi Jaime,

shouldn't the last bin run to infinity in pT? we only remove jets with a track above 100 GeV after jet finding.

adding Laura here also: @lhavener

[alibuild]

2e9186eb1888ec3b90d875d22db343ed594d51d2: approval required: 1 of @adriansev (Adrian Sevcenco), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @germainma (Marie Germain), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @lcunquei (Leticia Cunqueiro Mendez), @loizides (Constantinos Loizides), @matplo (Mateusz Ploskon), @mfasDa (Markus Fasel), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @raymondEhlers (Raymond James Ehlers), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @vkucera (Vit Kucera)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

Hi @raymondEhlers @mverwe @mfasDa a bit delayed since I was at a conference and having issues with my AliPhysics, but I have committed some changes based on Raymond and Marta's suggestion (thanks both). The one thing that may need a bit more explanation is Raymond's suggestion to have a more 'on-the-fly' selection of the period, I have added a comment on what I wasn't sure about

[alibuild]

aeb696c235756b23d28a517e7d4ec175fb5c3f7e: approval required: 1 of @adriansev (Adrian Sevcenco), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @germainma (Marie Germain), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @lcunquei (Leticia Cunqueiro Mendez), @loizides (Constantinos Loizides), @matplo (Mateusz Ploskon), @mfasDa (Markus Fasel), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @raymondEhlers (Raymond James Ehlers), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @vkucera (Vit Kucera)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

Hi @raymondEhlers , see my updated commit. I added something similar to what you suggested to automate the period and centrality selection (see one reply on your specific comment about the centrality selection above). I needed to add back one flag to say whether to apply the pt-dependent efficiency. I also load the YAML file as you suggested - note this is done whether using the pt-dependent efficiency or not since it is initialised in the AddTask , i.e. before the pt-dependent uncertainty option would be enabled. Local tests on pp and embedded Pb-Pb run as expected. Let me know what you think anyway, thanks!

[alibuild]

f0cba582c973e7208611fd09b50f80f51f03f7c0: approval required: 1 of @adriansev (Adrian Sevcenco), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @germainma (Marie Germain), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @lcunquei (Leticia Cunqueiro Mendez), @loizides (Constantinos Loizides), @matplo (Mateusz Ploskon), @mfasDa (Markus Fasel), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @raymondEhlers (Raymond James Ehlers), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @vkucera (Vit Kucera)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

Thanks Raymond, I made the changes as you suggested

[raymondEhlers]

+1

[alibuild]

f0cba582c973e7208611fd09b50f80f51f03f7c0: approved: will be automatically merged on successful tests

[TimoWilken]

Hi @jaimenorman @raymondEhlers, this PR is marked as "draft" so it will not be tested. If it's actually ready to be merged, please press the "Ready for review" button in the GitHub web UI.

[jaimenorman]

Thanks Timo for spotting this, I just marked as ready for review

On 22 Sep 2022, at 10:31, Timo Wilken @.@.>> wrote:

Hi @jaimenormanhttps://github.com/jaimenorman @raymondEhlershttps://github.com/raymondEhlers, this PR is marked as "draft" so it will not be tested. If it's actually ready to be merged, please press the "Ready for review" button in the GitHub web UI.

— Reply to this email directly, view it on GitHubhttps://github.com/alisw/AliPhysics/pull/20727#issuecomment-1254769217, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGKGX4Q3RRXFA77XEJGRRN3V7QRN3ANCNFSM53S5YWEQ. You are receiving this because you were mentioned.Message ID: @.***>

[alibuild]

Error while checking build/AliPhysics/root6 for f0cba582c973e7208611fd09b50f80f51f03f7c0 at 2022-09-22 12:26:

## sw/BUILD/AliPhysics-latest/log
/sw/SOURCES/AliPhysics/20727-slc7_x86-64/0/PWG/Tools/AliYAMLConfiguration.h:6:10: fatal error: 'yaml-cpp/yaml.h' file not found
Error: /sw/slc7_x86-64/ROOT/v6-26-04-patches-alice1-24/bin/rootcint: compilation failure (/sw/BUILD/29240bef1030a8100c8c1986d9e4b93fef7aa27f/AliPhysics/PWGDQ/dielectronJet/G__PWGDQdielectronJetbaad31b72f_dictUmbrella.h)
gmake[2]: *** [PWGDQ/dielectronJet/libPWGDQdielectronJet.rootmap] Error 1
gmake[1]: *** [PWGDQ/dielectronJet/CMakeFiles/PWGDQdielectronJet-object.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....
gmake: *** [all] Error 2

Full log here.

[jaimenorman]

@raymondEhlers I see that the build check wasn't successful - actually I saw this error once when testing, and may have fixed by adding 'yaml-cpp/yaml.h' to the jet task, though after your suggestion to remove the build was OK, so I'm not exactly sure what causes it to fail here. Do you have any ideas?

[raymondEhlers]

This is mysterious to me since I use the same class in my Dynamical Grooming class and it's included right next to AliEmcalJetTask in the CMakeLists, so should have the same includes. Will take a closer look in a bit

[raymondEhlers]

Figured it out - it's a downstream problem. The issue is that PWGDQ/dielectronJet/AliAnalysisTaskJpsiJet.h includes AliEmcalJetTask.h, but doesn't add the yaml include directory in the CMakeLists. Note that the full log is:

In file included from input_line_9:15:
In file included from /sw/SOURCES/AliPhysics/20727-slc7_x86-64/0/PWGDQ/dielectronJet/AliAnalysisTaskJpsiJet.h:46:
In file included from /sw/SOURCES/AliPhysics/20727-slc7_x86-64/0/PWGJE/EMCALJetTasks/AliEmcalJetTask.h:45:
/sw/SOURCES/AliPhysics/20727-slc7_x86-64/0/PWG/Tools/AliYAMLConfiguration.h:6:10: fatal error: 'yaml-cpp/yaml.h' file not found
#include <yaml-cpp/yaml.h>

I'm not quite sure how that works, since the directory is included in our CMakeLists.txt, and it's a hard requirement. I thought PWGs weren't supposed to cross link, but it's fine. There's two to fix it:

1. Add the yaml include directory to their CMakeLists.txt. ie. Add ${AliPhysics_SOURCE_DIR}/PWG/Tools/yaml-cpp/include after https://github.com/alisw/AliPhysics/blob/1b1c1d94ea528856891861a38849b31ab2033c79/PWGDQ/dielectronJet/CMakeLists.txt#L50
2. Alternatively, we could move #include "AliEmcalJetTask.h" from the .h to the .cxx since it doesn't need it in the header. I guess that still may require the CMakeLists edit though

I would do option 1 since it's easier, I think. I would do it myself, but I can't commit to your PR. Jaime, could you please add it? We'll have to get their approval, but it should be fixed on their end, so it's fine (it's basically the same as fj - you can access it via a wrapper, but if you want to use it, you need to add the includes and libs)

[alibuild]

df281aa5bff129e05c2fa959d0e180d6856efb44: approval required: 1 of @atoia (Alberica Toia), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @dsekihat (Daiki Sekihata), @hmurakam (Hikari Murakami), @iarsene (Ionut Cristian Arsene), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @otonvd (Oton Vazquez Doce), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @rbailhac (Raphaelle Bailhache), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @tkgunji (Taku Gunji), @wiechula (Jens Wiechula); 1 of @adriansev (Adrian Sevcenco), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @germainma (Marie Germain), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @lcunquei (Leticia Cunqueiro Mendez), @loizides (Constantinos Loizides), @matplo (Mateusz Ploskon), @mfasDa (Markus Fasel), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @raymondEhlers (Raymond James Ehlers), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @vkucera (Vit Kucera)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

thanks Raymond, I added option 1 as you suggest. We can wait for one of the DQ approvers to add their +1

[raymondEhlers]

+1 (also needs it from me, I think) thanks!

[alibuild]

df281aa5bff129e05c2fa959d0e180d6856efb44: approval required: 1 of @atoia (Alberica Toia), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @dsekihat (Daiki Sekihata), @hmurakam (Hikari Murakami), @iarsene (Ionut Cristian Arsene), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @otonvd (Oton Vazquez Doce), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @rbailhac (Raphaelle Bailhache), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @tkgunji (Taku Gunji), @wiechula (Jens Wiechula)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

[jaimenorman]

@atoia @chiarazampolli @davidrohr @dsekihat @hmurakam @iarsene @jgrosseo @ktf @otonvd @pzhristov @qgp @rbailhac @sawenzel @shahor02 @tkgunji @wiechula could we get a +1 from the DQ side? we just added a directory to the CMakeLists.txt to fix a dependency error

[iarsene]

+1

[alibuild]

df281aa5bff129e05c2fa959d0e180d6856efb44: approved: will be automatically merged on successful tests