StJetMaker: add idTruth to StJetTrack #152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plexoos
approved these changes
Sep 22, 2021
Raise the version number by 1 for StjTrack
zlchang
approved these changes
Sep 22, 2021
veprbl
commented
Sep 22, 2021
| @@ -57,7 +57,7 @@ class StjTrack : public TObject { | |||
| double nSigmaTofProton; | |||
| double nSigmaTofElectron; | |||
|
|
|||
| ClassDef(StjTrack,5); | |||
| ClassDef(StjTrack,6); | |||
There was a problem hiding this comment.
This should never appear on disk, so really should be a version=0
There was a problem hiding this comment.
but changing that is not the point of this PR
|
This is a very old code maybe from Tai Sakuma. This has never been used since 2009 analysis, but I remembered before the 2009 analysis tracks are saved in this StjTrack format in the jet tree.
On Sep 22, 2021, at 1:39 PM, Dmitry Kalinkin ***@***.***> wrote:
@veprbl commented on this pull request.
________________________________
In StRoot/StJetMaker/tracks/StjTrackList.h<#152 (comment)>:
@@ -57,7 +57,7 @@ class StjTrack : public TObject {
double nSigmaTofProton;
double nSigmaTofElectron;
- ClassDef(StjTrack,5);
+ ClassDef(StjTrack,6);
but changing that is not the point of this PR
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3KXVMK2NMSDZEI5ONTUDIIG3ANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
It is old, but looks like the previous tree format didn't use streamers |
|
There should be a reason why it started at 1. For now we don’t this any more. There is no harm to increment it each time the class got updated.
…Sent from my iPhone
On Sep 22, 2021, at 2:12 PM, Dmitry Kalinkin ***@***.***> wrote:
This is a very old code maybe from Tai Sakuma. This has never been used since 2009 analysis, but I remembered before the 2009 analysis tracks are saved in this StjTrack format in the jet tree.
It is old, but looks like the previous tree format didn't use streamers
https://github.com/star-bnl/star-sw/blob/e0f02802d3ac24161117286395bf64f007cae923/StRoot/StJetMaker/tree/StjTrackListWriter.cxx
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3PH7376L4BBCTMHUEDUDIMADANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@veprbl do you want to send an email to spin pwg to let people know that you have updated the code or do you want me to do it? My concern is that now people start using code from dev, today they submit job with one code and the next day, they would submit the same job without realizing using a new code. Once you sent the email, what about we wait for one or two days on the list and see if people have objections? If none, then we will merge the branch. |
|
Regarding merging this, I'm fine with whatever you decide, @zlchang |
What don't you write an email to the pwg, just explain what the two variables are (idtrue and qatrue) and their potential impact to your analysis? If no objections, we will merge the code in 24 hours. |
|
Hello All, I was wondering if these new track properties need to be added to StUEMaker2009.cxx to make sure that the tracks from the UE region trees have access to these properties and are consistent with the off-axis cone and jet trees? |
|
Good point! StUEMaker2009 is Grant Webb’s region underlying event tree maker. I don’t think anyone else has used this maker except him. We should add the track information to enssure consistency for all the code.
On Sep 23, 2021, at 8:51 AM, BassamAboona ***@***.***> wrote:
Hello All,
I was wondering if these new track properties need to be added to StUEMaker2009.cxx to make sure that the tracks from the UE region trees have access to these properties and are consistent with the off-axis cone and jet trees?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3K36N7KZLGEYF7OZYDUDMPFBANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
The region code was never used and contains an unfixed bug. The PicoDst trees are better than it in many respects. I will pass on implementing this suggestion. |
|
I think some people in spin pwg still generate region trees using the macro that Grant had used. It’s just a few lines change and should be easy to do. Bassam added the TOF information to this UE maker and it is better to be consistent. Overall this update is not just for your analysis and it should benefit other people’s too.
On Sep 23, 2021, at 8:38 PM, Dmitry Kalinkin ***@***.***> wrote:
The region code was never used and contains an unfixed bug. The PicoDst trees are better than it in many respects. I will pass on implementing this suggestion.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3MAHCCFFTOBHAT72RDUDPCBLANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Hi Zilong and Dmitry,
For my analysis, I do produce and use the use region trees.
I am curious, what is the bug you are referring to in the UE region codes, Dmitry?
Best,
Bassam Aboona
On Sep 23, 2021, at 7:53 PM, Zilong Chang ***@***.***> wrote:
I think some people in spin pwg still generate region trees using the macro that Grant had used. It’s just a few lines change and should be easy to do. Bassam added the TOF information to this UE maker and it is better to be consistent. Overall this update is not just for your analysis and it should benefit other people’s too.
On Sep 23, 2021, at 8:38 PM, Dmitry Kalinkin ***@***.***> wrote:
The region code was never used and contains an unfixed bug. The PicoDst trees are better than it in many respects. I will pass on implementing this suggestion.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3MAHCCFFTOBHAT72RDUDPCBLANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/star-bnl/star-sw/pull/152*issuecomment-926265987__;Iw!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8KesU5Va6g$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AUK2XR477RIN7F6VHU3DFITUDPD2FANCNFSM5EPB7AEQ__;!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8KdxKczd9Q$>.
Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8Kd0xBteSQ$> or Android<https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8KdMkdJnrg$>.
|
I advise that you don't. It's a waste of disk space to have those.
Actually, there are two bugs, both unresolved: |
|
We can get a another pull request to fix those bugs, but to be consistent, Dmitry why don’t you add those two lines in Grant’s UE maker and move on with request.
On Sep 23, 2021, at 9:08 PM, Dmitry Kalinkin ***@***.***> wrote:
For my analysis, I do produce and use the use region trees.
I advise that you don't. It's a waste of disk space to have those.
I am curious, what is the bug you are referring to in the UE region codes, Dmitry?
Actually, there are two bugs, both unresolved:
https://drupal.star.bnl.gov/STAR/system/files/jet_meeting_2017_02_01_v2.pdf
There was also bug in the only analysis that used those trees:
https://drupal.star.bnl.gov/STAR/blog/veprbl/run12-pp200-region-ue-plots-qa
hence, in the end, there is no physics yield from this software
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3JBHXFDI6OATM3F4GLUDPFR5ANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Pass |
|
Are you going to add or not? Then we are hanging on this request??
…Sent from my iPhone
On Sep 23, 2021, at 9:24 PM, Dmitry Kalinkin ***@***.***> wrote:
Pass
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3KOCBHMJUU36TDETCTUDPHMJANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
veprbl
commented
Sep 24, 2021
Comment on lines
+111
to
+112
| int mIdTruth; | ||
| int mQaTruth; |
There was a problem hiding this comment.
Suggested change
| int mIdTruth; | |
| int mQaTruth; | |
| unsigned short mIdTruth; | |
| unsigned short mQaTruth; |
https://www.star.bnl.gov/webdata/dox/html/classStMuTrack.html
StMuTrack::mIdTruth is UShort_t while StMuTrack::idTruth() is Int_t
veprbl
commented
Sep 24, 2021
Comment on lines
+238
to
+240
|
|
||
| track->mIdTruth = t.idTruth; | ||
| track->mQaTruth = t.qaTruth; |
There was a problem hiding this comment.
If you knew that this was what you wanted, just post a patch next time. Please.
|
This is what you needed to do!
Sent from my iPhone
On Sep 24, 2021, at 12:22 AM, Dmitry Kalinkin ***@***.***> wrote:
@veprbl commented on this pull request.
________________________________
In StRoot/StJetMaker/StUEMaker2009.cxx<#152 (comment)>:
+
+ track->mIdTruth = t.idTruth;
+ track->mQaTruth = t.qaTruth;
If you knew that this was what you wanted, just post a patch next time. Please.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3IFPQLDA7CFKFEYRV3UDP4J3ANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
You also need to modify stjtracklist
Sent from my iPhone
On Sep 24, 2021, at 12:22 AM, Dmitry Kalinkin ***@***.***> wrote:
@veprbl commented on this pull request.
________________________________
In StRoot/StJetMaker/StUEMaker2009.cxx<#152 (comment)>:
+
+ track->mIdTruth = t.idTruth;
+ track->mQaTruth = t.qaTruth;
If you knew that this was what you wanted, just post a patch next time. Please.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#152 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGH7F3IFPQLDA7CFKFEYRV3UDP4J3ANCNFSM5EPB7AEQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Are we good with recent commits? Keeping the idtruth and qatruth as int is fine to me because int has as twice or equal much storage bits as short. |
I don't think we need to have narrow type for stjtrack, it's not written to disk. |
|
Keeping int for stjettrack should be also fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should be useful for studies of simulation (e.g. select real tracks vs pile-up), to match tracks to the Pythia particles. For matching to secondary particles an external geant.root or minimc.root would be required.