Skip to content

TPC PID Skim Tree Creation#1002

Merged
jgrosseo merged 12 commits into
AliceO2Group:masterfrom
AnnalenaKa:branch_skimscreation2
Aug 10, 2022
Merged

TPC PID Skim Tree Creation#1002
jgrosseo merged 12 commits into
AliceO2Group:masterfrom
AnnalenaKa:branch_skimscreation2

Conversation

@AnnalenaKa

Copy link
Copy Markdown
Contributor

This pull request will add a check in the TPC PID Response class whether a tpc signal was present and the default values are changed to -999 for the resolution and number of sigma and to 0 for the expected signal.

In addition, a column with the expected signal is added to the TPC pid table, which is produced by the pid-tpc-full task.

Furthermore, the task DPG/Tasks/TPC/tpcSkimsTableCreator.cxx will allow to create a table, which will be used to train the neural network that is used for the multidimensional correction of the TPC response. The task is based on V0 and gamma decays plus TPC and TOF track selections at low momentum.

@AnnalenaKa

Copy link
Copy Markdown
Contributor Author

Hi,
@jezwilkinson , @ChSonnabend and @njacazio.
I have created the draft pull request adding the following changes:

  1. Some hasTPC() checks in the TPCPIDResponse.h class. Furthermore, the default values were changed to -999 for the sigma and nsigma and to 0 for the expected signal.

  2. I had to add the expected signal to the tpc table which is produced by the pid-tpc-full task. It was stored for the TOF but not for the TPC and we need it for our tree creation. Otherwise we will have to copy parts of the tpc full task in order to get the expected signal. If Nicolo, you don't agree with that change I can revert that part and change the way we get the expected signal for the tree creation and get the response from the CCDB or a file in addition to running the pid-tpc-full task.

  3. I added the skim tree creation task under DPG/Tasks/TPC. Please let me know if you want me to add some modifications.

Comment thread Common/Core/PID/TPCPIDResponse.h Outdated
Comment thread Common/Core/PID/TPCPIDResponse.h
Comment thread Common/Tools/handleParamTPCResponse.cxx Outdated
Comment thread Common/Core/PID/TPCPIDResponse.h
@jezwilkinson

Copy link
Copy Markdown
Contributor

Thanks for the commit! I had a couple of comments on the changes to the TPC response - having the NumberOfSigmas return -999 in case of an invalid signal is good, but i think returning -999 on the resolution could have unintended effects if it's not then checked for by the other functions that use it

@wiechula wiechula left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor comment: Please carefully check which variables can be declared const.

Comment thread Common/TableProducer/PID/pidTPC.cxx Outdated
Comment thread Common/TableProducer/PID/pidTPCFull.cxx Outdated
Comment thread Common/TableProducer/PID/pidTPCML.h
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated

@njacazio njacazio 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.

Hi @AnnalenaKa this looks super nice indeed! I have just some minor comments/suggestions.
Did you estimate the compression factor of the skimmed data WRT the original AODs?

Also I'm wondering if you are interested into carrying out some down-sampling at some point.

Comment thread Common/Core/PID/TPCPIDResponse.h Outdated
Comment thread Common/Core/PID/TPCPIDResponse.h Outdated
Comment thread Common/Core/PID/TPCPIDResponse.h
Comment thread Common/DataModel/PIDResponse.h Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
@AnnalenaKa

AnnalenaKa commented Jul 21, 2022

Copy link
Copy Markdown
Contributor Author

Hi,
@jezwilkinson @wiechula and @njacazio
I have included all your comments I received here or in the meeting. Please let me know if you have additional wishes or comments. If everyone is satisfied with the changes I would mark the request as ready for review. As you can see I included also the event selection and track selection in the skimming task.

@njacazio

Copy link
Copy Markdown
Collaborator

Thanks a lot @AnnalenaKa very nice, all good from me

@wiechula wiechula left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some more extremely minor suggestions.
Also I would prefer to explicitly use the std:: math functions:

pow,TMath::Power -> std::pow
abs,TMath::Abs -> std::abs
sqrt -> std::sqrt

etc.

Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
Comment thread DPG/Tasks/TPC/tpcSkimsTableCreator.cxx Outdated
@jezwilkinson

Copy link
Copy Markdown
Contributor

All looks good to me, thanks for the updates!

@AnnalenaKa AnnalenaKa marked this pull request as ready for review July 21, 2022 13:33
mfaggin
mfaggin previously approved these changes Jul 21, 2022

@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.

I see that this PR was hugely discussed already, green light from my side then.

@njacazio

Copy link
Copy Markdown
Collaborator

I would just wait #1025 to be merged

wiechula
wiechula previously approved these changes Jul 21, 2022

@wiechula wiechula left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks a lot @AnnalenaKa looks good!

@AnnalenaKa AnnalenaKa force-pushed the branch_skimscreation2 branch from c5ec853 to 42b26fc Compare July 28, 2022 14:37
@AnnalenaKa

Copy link
Copy Markdown
Contributor Author

I would just wait #1025 to be merged

Hi @njacazio I saw this pull request was merged. Could we proceed with the review now to merge this PR as well?

@njacazio

Copy link
Copy Markdown
Collaborator

I would just wait #1025 to be merged

Hi @njacazio I saw this pull request was merged. Could we proceed with the review now to merge this PR as well?

Sure, for me it's already fine! @jgrosseo can we have this merged?

@njacazio njacazio requested a review from ddobrigk as a code owner August 6, 2022 08:24
@njacazio

njacazio commented Aug 6, 2022

Copy link
Copy Markdown
Collaborator

I fixed a conflict brought in by a recent PR merge that prevented the merge

@njacazio njacazio enabled auto-merge (squash) August 6, 2022 08:40

@ddobrigk ddobrigk 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.

Stalled PR, looks ok and reviewed, approved

@njacazio njacazio disabled auto-merge August 7, 2022 05:30
@njacazio njacazio enabled auto-merge (squash) August 7, 2022 05:30
@jgrosseo jgrosseo disabled auto-merge August 10, 2022 14:44
@jgrosseo jgrosseo merged commit 516234c into AliceO2Group:master Aug 10, 2022
@AnnalenaKa AnnalenaKa deleted the branch_skimscreation2 branch October 17, 2022 07:39
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.

8 participants