Skip to content

PWGHF: Make transition from McParticles_000 to McParticles_001#510

Merged
jgrosseo merged 21 commits into
AliceO2Group:masterfrom
fgrosa:mcparticle
Feb 23, 2022
Merged

PWGHF: Make transition from McParticles_000 to McParticles_001#510
jgrosseo merged 21 commits into
AliceO2Group:masterfrom
fgrosa:mcparticle

Conversation

@fgrosa
Copy link
Copy Markdown
Collaborator

@fgrosa fgrosa commented Feb 7, 2022

@jgrosseo @vkucera @fcolamar please double check that I did not mess up anything. Thanks a lot!

Comment thread Common/Core/RecoDecay.h Outdated
Comment thread PWGHF/TableProducer/HFCandidateCreator3Prong.cxx Outdated
Comment thread Common/Core/RecoDecay.h Outdated
int indexDaughterLast = particleMother.daughtersIds().back(); // index of the last direct daughter
if ((indexDaughterFirst > -1 && indexDaughterLast > -1)) {
std::vector<int> arrAllDaughtersIndex;
RecoDecay::getDaughters(particlesMC, particleMother, &arrAllDaughtersIndex, array{(int)(kGamma), (int)(pdg::Code::kJpsi)}, 1);
Copy link
Copy Markdown
Contributor

@jgrosseo jgrosseo Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As just discussed in the meeting: Replace by just has_daughters(). The index values themselves are not used elsewhere

Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread PWGHF/Tasks/HFMCValidation.cxx Outdated
Comment thread PWGHF/Tasks/taskD0parametrizedPID.cxx Outdated
Comment thread PWGHF/Tasks/taskD0parametrizedPID.cxx
@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Feb 10, 2022

Actually, there are a few more places in RecoDecay that would benefit from replacement with .has_daughters() and .daughters().size().

@fgrosa
Copy link
Copy Markdown
Collaborator Author

fgrosa commented Feb 10, 2022

Actually, there are a few more places in RecoDecay that would benefit from replacement with .has_daughters() and .daughters().size().

Should be done everywhere now!

@fgrosa
Copy link
Copy Markdown
Collaborator Author

fgrosa commented Feb 10, 2022

Further improvements foreseen after this PR AliceO2Group/AliceO2#8113 gets merged

@fgrosa
Copy link
Copy Markdown
Collaborator Author

fgrosa commented Feb 11, 2022

@jgrosseo @vkucera the PR should be now updated using the methods implemented in AliceO2Group/AliceO2#8113 and rawIteratorAt() instead of iteratorAt()

jgrosseo
jgrosseo previously approved these changes Feb 11, 2022
@vkucera vkucera changed the title [PWGHF]: Make transition from McParticles_000 to McParticles_001 PWGHF: Make transition from McParticles_000 to McParticles_001 Feb 12, 2022
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h Outdated
Copy link
Copy Markdown
Collaborator

@vkucera vkucera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine. Just please fix the removed comments.

Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h Outdated
Copy link
Copy Markdown
Collaborator

@vkucera vkucera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine. Just please fix the removed comments.

Copy link
Copy Markdown
Collaborator

@vkucera vkucera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Fabrizio!
Please verify that the MC plots from the validation framework match the reference.

jgrosseo
jgrosseo previously approved these changes Feb 16, 2022
Comment thread Common/Core/RecoDecay.h Outdated
Comment on lines 674 to 676
for (auto& dau : particle.template daughters_as<typename std::decay_t<T>::parent_t>()) {
getDaughters(dau, list, arrPDGFinal, depthMax, stage);
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkucera when this piece of code is executed I get the following error:
[ERROR] Exception caught: Unable to find column with label fIndexMcCollisions
even if I checked and the column fIndexMcCollisions is present in the O2mccollisionlabel table in the AO2D file that I am analysing.
I wrote this piece of code following what was suggested by @aalkin for
particleMother = particleMother.template mothers_first_as<typename std::decay_t<T>::parent_t>();
however I am not sure that it works also in this case. I also tried by replacing typename std::decay_t<T>::parent_t with simply aod::McParticles but I get the same error. @aalkin @jgrosseo do you have any suggestion? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fgrosa this particular exception is raised when constructing a table. When the slice index is unset it is supposed to return empty table. It may happen that this actually creates an empty table with no schema and thus the construction fails. For now you can protect this piece of code with check for has_daughters().

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @aalkin thanks! Unfortunately even if I put the protection:

    if (particle.has_daughters()) {
      for (auto& dau : particle.template daughters_as<typename std::decay_t<T>::parent_t>()) {
        getDaughters(dau, list, arrPDGFinal, depthMax, stage);
      }
    }

it keeps crashing with the same error. Do you have any other idea by chance?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to debug this, I assume the line auto dauTable = particle.template daughters_as<typename std::decay_t<T>::parent_t>(); would crash as well, could you check?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for has_daughters() has no effect here because this line is reached only if has_daughters() returned true.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aalkin indeed it crashes as well. @vkucera right because if isFinal is true and has_daughters() is false it goes through the first if statement, but the it returns anyway before this part of the code.
I think that one possible solution would be to loop over indices in this case instead of getting the sliced table of daughters, but it would require to pass again the MC particle table as argument. However I have no better ideas at the moment. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aalkin can you have a look?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aalkin Would this work?

  template <std::size_t N, typename T>
  static void getDaughters(const typename T::iterator& particle,
    ...
    for (auto& dau : particle.daughters_as<T>()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkucera I doubt this will be able to deduce type automatically from a function call, but it is better to check explicitly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkucera @aalkin I already tried this, but the compilation fails with the following error: couldn't deduce template parameter 'T'

@fgrosa
Copy link
Copy Markdown
Collaborator Author

fgrosa commented Feb 23, 2022

Hi @jgrosseo @vkucera @aalkin with my last commit I reverted the code to the usage of daughtersIds() and mothersIds() in RecoDecay.h instead of mothers_first_as<> and daughters_as<> since I did not find a way to make it work otherwise. With this implementation I get the same result as compared to the reference efficiency in https://github.com/AliceO2Group/Run3Analysisvalidation/blob/master/codeHF/MC_d0_eff_ref.pdf so if you agree I would proceed with this now in the seek of moving forward (but indeed it would be good to understand why I did not manage to make it work otherwise). What do you think? Thanks a lot!

@jgrosseo
Copy link
Copy Markdown
Contributor

I agree to move ahead and merge this. Could you subsequently, prepare a PR where you put mothers_first_as and daughters_as in a single location, such that @aalkin could follow up on it?

jgrosseo
jgrosseo previously approved these changes Feb 23, 2022
@jgrosseo jgrosseo marked this pull request as ready for review February 23, 2022 13:54
@jgrosseo
Copy link
Copy Markdown
Contributor

Waiting for @vkucera approval of course...

Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread Common/Core/RecoDecay.h Outdated
Comment thread PWGHF/Tasks/taskXicc.cxx Outdated
fgrosa and others added 2 commits February 23, 2022 16:19
Co-authored-by: Vít Kučera <vit.kucera@cern.ch>
Co-authored-by: Vít Kučera <vit.kucera@cern.ch>
@fgrosa
Copy link
Copy Markdown
Collaborator Author

fgrosa commented Feb 23, 2022

I agree to move ahead and merge this. Could you subsequently, prepare a PR where you put mothers_first_as and daughters_as in a single location, such that @aalkin could follow up on it?

Indeed this is a good idea @jgrosseo to keep working on it. I will open a new PR as soon as we merge this one

Comment on lines +439 to +454
if (rightDecayChannels) { //fill with D and Dbar daughter particls acceptance checks
bool candidate1DauInAcc = true;
bool candidate2DauInAcc = true;
for (auto& dau : particle1.daughters_as<MCParticlesPlus>()) {
if (std::abs(dau.eta()) > etaCut) {
candidate1DauInAcc = false;
break;
}
}
for (auto& dau : particle2.daughters_as<MCParticlesPlus>()) {
if (std::abs(dau.eta()) > etaCut) {
candidate2DauInAcc = false;
break;
}
}
if (candidate1DauInAcc && candidate2DauInAcc && particle1.pt() > ptCut && particle2.pt() > ptCut) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fcolamar Can you please check these changes in your tasks and confirm that you are fine with them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants