Skip to content

PWGHF: first checked implementation of Xic->pKpi filtering#5436

Merged
jgrosseo merged 14 commits intoAliceO2Group:devfrom
mfaggin:mfaggin_XicTopKpi
Mar 10, 2021
Merged

PWGHF: first checked implementation of Xic->pKpi filtering#5436
jgrosseo merged 14 commits intoAliceO2Group:devfrom
mfaggin:mfaggin_XicTopKpi

Conversation

@mfaggin
Copy link
Copy Markdown
Contributor

@mfaggin mfaggin commented Feb 10, 2021

No description provided.

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 10, 2021

Linked PR in validation framework: AliceO2Group/Run3AnalysisValidation#158

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 12, 2021

@ginnocen @vkucera @DelloStritto few required fixes implemented

@mfaggin mfaggin marked this pull request as ready for review February 16, 2021 09:22
@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 16, 2021

@ginnocen switched to "ready for review" status

Comment thread Analysis/DataModel/include/AnalysisDataModel/HFSecondaryVertex.h
Comment thread Analysis/Tasks/PWGHF/HFXicCandidateSelector.cxx Outdated
Comment thread Analysis/DataModel/include/AnalysisDataModel/HFCandidateSelectionTables.h Outdated
@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Feb 18, 2021

@mfaggin @DelloStritto @ginnocen If I understand correctly, the only difference between the Lc and Xic selector should eventually be the configuration (including the mass cut) and the output flag column.
Why don't we try to reuse as many functions as possible from the existing Lc selector?

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 18, 2021

@mfaggin @DelloStritto @ginnocen If I understand correctly, the only difference between the Lc and Xic selector should eventually be the configuration (including the mass cut) and the output flag column.
Why don't we try to reuse as many functions as possible from the existing Lc selector?

IMHO, this should be avoided at this stage. My implementation of the Xic is indeed based on the Lc task, since we are talking about particles that are reconstructed in the same decay channel. However, being two different particles with different kinematics, I would not force them to be equal or strongly correlated for the moment, because for real analysis things may differentiate. I agre with you for methods like the one on the pKpi invariant mass calculations, but not for all methods.

@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Feb 18, 2021

@mfaggin Is there any topological cut that could ever be applied only for Lc and not for Xic?

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 18, 2021

@vkucera at this stage I do not see indeed, but I do not have such a long-time view to confirm it at 100%. I can try to have a look if such an operation would be too constraining or not, but I do not have too much time to dedicate on this business in a short time-scale. Do you think it's crucial for this PR to be approved? I may think to address this topic after this PR merging, so that something is already in place for Xic.

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 22, 2021

Hi @vkucera , I see conflicts in Analysis/Tasks/PWGHF/CMakeLists.txt that probably depend on the fact that in the meanwhile something changed in the dev branch (also for the validation framework). I think it's better if you fix it as you prefer, according to the new conventions, in both cases. Do you agree?

@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Feb 22, 2021

Hi @vkucera , I see conflicts in Analysis/Tasks/PWGHF/CMakeLists.txt that probably depend on the fact that in the meanwhile something changed in the dev branch (also for the validation framework). I think it's better if you fix it as you prefer, according to the new conventions, in both cases. Do you agree?

Hi @mfaggin , those should be just conflicts due to the addition of the MC validation task. You should just rebase, resolve the conflicts and push again.

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Feb 23, 2021

Hi @vkucera , I see conflicts in Analysis/Tasks/PWGHF/CMakeLists.txt that probably depend on the fact that in the meanwhile something changed in the dev branch (also for the validation framework). I think it's better if you fix it as you prefer, according to the new conventions, in both cases. Do you agree?

Hi @mfaggin , those should be just conflicts due to the addition of the MC validation task. You should just rebase, resolve the conflicts and push again.

Hi @vkucera , I manually resolved the conflicts directly from the git interface, please check if my implementations are ok.

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.

As discussed, the selector should use functions from the Lc selector, but that can be done in the next steps.

Comment thread Analysis/Tasks/PWGHF/HFCandidateCreator3Prong.cxx Outdated
Comment thread Analysis/Tasks/PWGHF/HFCandidateCreator3Prong.cxx Outdated
Comment thread Analysis/Tasks/PWGHF/HFXicToPKPiCandidateSelector.cxx Outdated
Comment thread Analysis/Tasks/PWGHF/taskXic.cxx Outdated
 - (HFCandidateCreator3Prong.cxx) remove refuse std::move, forgot in the middle of the for loop
 - (HFCandidateCreator3Prong.cxx) fix if statement condition for isMatchedMCGen
 - (taskXic.cxx) fix MC histogram titles
@mfaggin mfaggin force-pushed the mfaggin_XicTopKpi branch from 1b521b4 to d79b31b Compare March 1, 2021 08:11
Comment thread Analysis/Tasks/PWGHF/HFCandidateCreator3Prong.cxx Outdated
@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Mar 1, 2021

The code looks fine now.
Just the variable names need to be fixed to comply with the conventions.

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Mar 2, 2021

The code looks fine now.
Just the variable names need to be fixed to comply with the conventions.

Hi @vkucera , which variables are you referring to? If I am not wrong, the variable names in the HFXicToPKPiCandidateSelector.cxx are equal to the ones in HFLcToPKPiCandidateSelector.cxx, but I am not sure if you are referring to them.

@vkucera
Copy link
Copy Markdown
Collaborator

vkucera commented Mar 2, 2021

The code looks fine now.
Just the variable names need to be fixed to comply with the conventions.

Hi @vkucera , which variables are you referring to? If I am not wrong, the variable names in the HFXicToPKPiCandidateSelector.cxx are equal to the ones in HFLcToPKPiCandidateSelector.cxx, but I am not sure if you are referring to them.

I meant the configurables and histograms, but it's fine, we can rename them later, together with everything else.

vkucera
vkucera previously approved these changes Mar 2, 2021
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.

@ginnocen Good from my side.

@mfaggin
Copy link
Copy Markdown
Contributor Author

mfaggin commented Mar 4, 2021

@vkucera I followed your instructions and the adaptAnalysisTask and charge() changes should be correctly in place.

Comment thread Analysis/Tasks/PWGHF/HFXicToPKPiCandidateSelector.cxx Outdated
@mfaggin mfaggin force-pushed the mfaggin_XicTopKpi branch from 149f368 to e20d36b Compare March 5, 2021 08:34
Comment thread Analysis/Tasks/PWGHF/taskXic.cxx Outdated
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.

label -> mcParticle

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vkucera the change is in place

@mfaggin mfaggin force-pushed the mfaggin_XicTopKpi branch from 9d97ed8 to afb4535 Compare March 8, 2021 08:25
vkucera
vkucera previously approved these changes Mar 8, 2021
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.

Comment thread Analysis/Tasks/PWGHF/taskXic.cxx Outdated
Comment thread Analysis/Tasks/PWGHF/taskXic.cxx Outdated
@ginnocen
Copy link
Copy Markdown
Collaborator

ginnocen commented Mar 8, 2021

hi @mfaggin @vkucera. I added a couple of very minor comments. We will need to improve the selector, as you know :). Once those two little details are solved I think it is ready to go. Thanks to both for the nice work.

@mfaggin mfaggin force-pushed the mfaggin_XicTopKpi branch from 7b66c68 to a01c377 Compare March 8, 2021 15:26
@ginnocen ginnocen self-requested a review March 8, 2021 15:39
@jgrosseo
Copy link
Copy Markdown
Collaborator

Mac CI pending since 2 days. Merging.

@jgrosseo jgrosseo merged commit 2f27fc3 into AliceO2Group:dev Mar 10, 2021
@mfaggin mfaggin deleted the mfaggin_XicTopKpi branch March 17, 2021 09:50
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Nov 22, 2021
…oup#5436)

* PWGHF: first checked code of Xic->pKpi filtering

* Update HFSecondaryVertex.h

* Update HFXicCandidateSelector.cxx

* Fix code with camelCase syntax conventions

* Fix code syntax conventions for Xic methods

* PWGHF: further fixes for Xic to pKpi filtering and analysis

 - (HFCandidateCreator3Prong.cxx) remove refuse std::move, forgot in the middle of the for loop
 - (HFCandidateCreator3Prong.cxx) fix if statement condition for isMatchedMCGen
 - (taskXic.cxx) fix MC histogram titles

* PWGHF: avoid storing return values of RecoDecay::getMatchedMCRec and isMatchedMCGen for Xic->pKpi

* PWGHF: pair Xic code with adaptAnalysisTask and charge() changes

* PWGHF: remove check for charge to be different from 0

* PWGHF: change label_as into mcParticle_as in taskXic

* PWGHF: fix particle label for ct distribution in taskXic

* PWGHF: remove selection flag for Xicbar (obsolete) from taskXic

Co-authored-by: Mattia Faggin <mfaggin@cern.ch>
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.

5 participants