Skip to content

Extend DPG tools#1044

Merged
njacazio merged 4 commits into
AliceO2Group:masterfrom
njacazio:nj-performance
Jul 22, 2022
Merged

Extend DPG tools#1044
njacazio merged 4 commits into
AliceO2Group:masterfrom
njacazio:nj-performance

Conversation

@njacazio

@njacazio njacazio commented Jul 22, 2022

Copy link
Copy Markdown
Collaborator
  • more plots with different ev. time for TOF
  • Fixes in hist naming in qaEfficiency
  • extend qaPIDPTC
  • improve PDG selection in qa lite

@njacazio
Extend DPG tools
d9c0e57 
- more plots with different ev. time for TOF
- Fixes in hist naming in qaEfficiency
- extend qaPIDPTC
- improve PDG selection in qa lite
@njacazio

njacazio commented Jul 22, 2022

Copy link
Copy Markdown
Collaborator Author

@mfaggin @arossi81 could you please also have a look pls? I will do other changes later

mfaggin
mfaggin reviewed Jul 22, 2022
View reviewed changes

@mfaggin mfaggin left a comment

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.

ciao @njacazio I have just a couple of questions in qaEventTrackLite.cxx, no comments in qaEfficiency.cxx and for the PID qa ones I leave you complete freedom (I do not see any evident mistake though)

Comment thread DPG/Tasks/qaEventTrackLite.cxx Outdated

// MC selections
Configurable<int> pdgCodeSel{"pdgCodeSel", 2, "pdgCode based particle selection, 1 defines pi,K,p,mu,e, 2 all final-state charged particles including light (hyper)nuclei"};
Configurable<int> pdgCodeSel{"pdgCodeSel", 2, "pdgCode based particle selection, PDG code required for particles to have OR: 1 defines pi,K,p,mu,e, 2 all final-state charged particles including light (hyper)nuclei"};

@mfaggin mfaggin Jul 22, 2022

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.

what's OR?

@njacazio njacazio Jul 22, 2022

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.

it's not clear, you're right. The way it is implemented one can either select a PDG code or select 1 for pikapr or 2 for pikapr.... I will fix this

@mfaggin mfaggin Jul 22, 2022

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.

ah ok probably I misunderstood the comment string you changed, but it should be clear

Comment thread DPG/Tasks/qaEventTrackLite.cxx
return true;
if (pdgCodeSel == 2) {
if (abspdgcode == 3222 || abspdgcode == 3112 || abspdgcode == 3312 || abspdgcode == 3334 || abspdgcode == 1000010020 || abspdgcode == 1000010030 || abspdgcode == 1000020030 || abspdgcode == 1000020040 || abspdgcode == 1010010030 || abspdgcode == 1010020040)
switch (abspdgcode) {

@mfaggin mfaggin Jul 22, 2022

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.

my ignorance: are you sure that this switch statement does the same thing of the original if? Reading this code I naively interpret that return true; is done only for case 2212

@njacazio njacazio Jul 22, 2022

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.

it does the same, but now it's more clear, I also removed the abs value for the first selection

@mfaggin mfaggin Jul 22, 2022

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.

this is probably because there are no break statements anywhere and then it continues until return true;, anyway I trust you

mfaggin
mfaggin reviewed Jul 22, 2022
View reviewed changes
Comment thread DPG/Tasks/qaEventTrackLite.cxx Outdated

// MC selections
Configurable<int> pdgCodeSel{"pdgCodeSel", 2, "pdgCode based particle selection, PDG code required for particles to have OR: 1 defines pi,K,p,mu,e, 2 all final-state charged particles including light (hyper)nuclei"};
Configurable<int> pdgCodeSel{"pdgCodeSel", 2, "pdgCode based particle selection. Either provide a PDG code required for particles OR `1` defines pi,K,p,mu,e, `2` all final-state charged particles including light (hyper)nuclei"};

@mfaggin mfaggin Jul 22, 2022

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.

much more clear now, thank you

@njacazio
njacazio enabled auto-merge (squash) July 22, 2022 07:35
alibuild
alibuild previously approved these changes Jul 22, 2022
View reviewed changes

@alibuild alibuild left a comment

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.

Auto-approving on behalf of @njacazio.

mfaggin
mfaggin reviewed Jul 22, 2022
View reviewed changes

@mfaggin mfaggin left a comment

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.

This new solution is conceptually necessary, otherwise with the previous implementation one may also select particles with PDG code 1 or 2 (d,u quarks) and this can be potentially biasing for efficiency calculation @arossi81

@njacazio
njacazio disabled auto-merge July 22, 2022 08:07
@njacazio
njacazio enabled auto-merge (squash) July 22, 2022 08:07
alibuild
alibuild previously approved these changes Jul 22, 2022
View reviewed changes

@alibuild alibuild left a comment

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.

Auto-approving on behalf of @njacazio.

@alibuild

alibuild commented Jul 22, 2022

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/o2 for b735dc8 at 2022-07-22 10:47:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/1044-slc7_x86-64/0/DPG/Tasks/qaEventTrackLite.cxx:590:9: error: 'pdgCodeSelMode' was not declared in this scope; did you mean 'pdgCodeMode'?
ninja: build stopped: subcommand failed.

Full log here.

@njacazio
njacazio disabled auto-merge July 22, 2022 09:49
@njacazio
njacazio enabled auto-merge (squash) July 22, 2022 09:49
alibuild
alibuild approved these changes Jul 22, 2022
View reviewed changes

@alibuild alibuild left a comment

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.

Auto-approving on behalf of @njacazio.

@njacazio
njacazio merged commit 4ac46a4 into AliceO2Group:master Jul 22, 2022
@arossi81

arossi81 commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

This new solution is conceptually necessary, otherwise with the previous implementation one may also select particles with PDG code 1 or 2 (d,u quarks) and this can be potentially biasing for efficiency calculation @arossi81

Ciao @mfaggin, no there was not such possibility (not even for the purity) in the code (because or the loop was on reco tracks, or, for the loop over MC particles, because of the && with the request of the particle being physical primary) but it's ok the new implementation, it is more explicit overall.

@njacazio
njacazio deleted the nj-performance branch July 22, 2022 12:47
lbariogl pushed a commit to CF-tutorials/O2Physics that referenced this pull request Jul 28, 2022
@njacazio @lbariogl
Extend DPG tools (AliceO2Group#1044)
a2628c3 
- more plots with different ev. time for TOF
- Fixes in hist naming in qaEfficiency
- extend qaPIDPTC
- improve PDG selection in qa lite
  - Add pdg selection configurable for multiple particles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfaggin mfaggin mfaggin left review comments
@alibuild alibuild alibuild approved these changes
@mario-krueger mario-krueger Awaiting requested review from mario-krueger

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@njacazio @alibuild @arossi81 @mfaggin