[PWGJE] Updating jetD0AngSubstructure.cxx#15914
[PWGJE] Updating jetD0AngSubstructure.cxx#15914pdhankhe wants to merge 17 commits intoAliceO2Group:masterfrom
Conversation
pdhankhe
commented
Apr 22, 2026
pdhankhe
commented
- Fixed the formatting issues of columns
- Added task to match detector level and particle level Monte Carlo particles.
- Added an additional task to calculate the Monte Carlo efficiency.
- Using templated analyse functions to easily define process functions for other particles than D0 if desired.
Please consider the following formatting changes to AliceO2Group#15311
Please consider the following formatting changes to AliceO2Group#15311
- Added task to analyse D0 candidates from experimental data. Histograms for different observables such as angularity etc. are made for quick checking purposes. - Added task to match detector level and particle level Monte Carlo particles. - Added an additional task to calculate the Monte Carlo efficiency. - Using templated analyse functions to easily define processfunctions for other particles than D0 if desired.
[PWGJE] Updated jetD0AngSubstructure.cxx
Pushing the updates from dev branch to master
Updated code to obey style guidelines.
pdhankhe
requested review from
alibuild,
fjonasALICE,
maoyx,
mfasDa,
mhemmer-cern,
nzardosh and
raymondEhlers
as code owners
|
O2 linter results: ❌ 0 errors, |
github-actions
Bot
changed the title
|
|
||
| // | ||
| #include <PWGHF/Core/DecayChannels.h> | ||
| // |
There was a problem hiding this comment.
please rebase to the latest master to avoid changing includes if not needed
| /* | ||
| // Experimental Data (analyseDataChargedSubstructure) | ||
| */ | ||
| HNAME(ex_col); // Collision Counter |
There was a problem hiding this comment.
why do you define these here? What is the operational benefit?
There was a problem hiding this comment.
There are two main reasons I prefer to define the histogram name this way:
- using HNAME allows me to only insert the name of a histogram once. If I were to make a change it will be consistent and change both the variable name and string definition.
- the code is more readable to me and it is easy to add a new definition.
There was a problem hiding this comment.
Your macro just defines strings with the same name as the content. If you want to rename a histogram, you have to change all occurrences of that string anyway. It just creates another level of indirection, making the references actually longer than their content. How does that make anything easier or more readable?
There was a problem hiding this comment.
It is a preference, it prevents me making typos in the histogram names. I added this, because before I had string and found myself making mistakes when writing the names as a string. The intellisense I use recognises variable names, but not bare strings. This makes it a lot easier for me to change names and prevent typos.
So indeed, if I would change the variable name, I have to do that elsewhere in the code (ctrl+f makes this easy in combination with the namespace), but that is not the point I intended to make. You do not seem to tell me that it is not allowed, so I rather keep it this way since it fits my style of working.
There was a problem hiding this comment.
Well, I just gave you several reasons why I think it makes the code worse in both aspects that you seem to care about.
Can you please explain how this setup allows you to rename a histogram without changing all the string occurrences?
The point of preventing typos seems irrelevant to me because you are supposed to test your changes and an attempt to fill a non-existent histogram should throw a run time error.
| CandidatesMCP const& /*mcpCandidates*/, | ||
| aod::JetTracks const& tracks, | ||
| aod::JetParticles const& particles, | ||
| HistogramRegistry& registry) |
There was a problem hiding this comment.
the registry should be a member of your struct and accesible everywhere
There was a problem hiding this comment.
The registry is a member of the struct. I have process methods at the bottom where I pass it as an argument to the templated analyse methods.
- Currently I use only one registry, but I might want to add one later in my planned analysis.
- I prefer to make the methods independent of which particle I analyse (e.g. the tables and registry).
I am not aware of all requirements so if this is something that is not recommended or impractical according to your experience. I am happy to change it. Otherwise I rather leave it this way.
There was a problem hiding this comment.
What would you try to achieve by having multiple registries?
There was a problem hiding this comment.
When I intend to add a process method for other particle species. However, I realise that this would require a different pipeline of tasks anyway and therefore seperate outputs. Therefore I can just as well use one registry since it will never happen that in the same pipelen the task is used for both particle species.
There was a problem hiding this comment.
Even if you did have processing of multiple species in different process functions of the same task, I don't see how multiple registries would help you do anything that cannot be done with one.
vkucera
left a comment
vkucera
left a comment
There was a problem hiding this comment.
Don't break the includes.
I do not see how the includes are broken. Could you perhaps elaborate on this or send a link to the relevant part in the documentation? |
|
5 participants
