star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

StJetMaker: add idTruth to StJetTrack #152

Closed veprbl closed 2 years ago

veprbl commented 2 years ago

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.

zlchang commented 2 years ago

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.hhttps://github.com/star-bnl/star-sw/pull/152#discussion_r714169641:

@@ -57,7 +57,7 @@ class StjTrack : public TObject { double nSigmaTofProton; double nSigmaTofElectron;

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#discussion_r714169641, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3KXVMK2NMSDZEI5ONTUDIIG3ANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

veprbl commented 2 years ago

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

zlchang commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-925166073, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3PH7376L4BBCTMHUEDUDIMADANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

zlchang commented 2 years ago

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

veprbl commented 2 years ago

Regarding merging this, I'm fine with whatever you decide, @zlchang

zlchang commented 2 years ago

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.

BassamAboona commented 2 years ago

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?

zlchang commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-925787441, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3K36N7KZLGEYF7OZYDUDMPFBANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

veprbl commented 2 years ago

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.

zlchang commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-926261364, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3MAHCCFFTOBHAT72RDUDPCBLANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

BassamAboona commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-926261364, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3MAHCCFFTOBHAT72RDUDPCBLANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://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 GitHubhttps://urldefense.com/v3/__https://github.com/star-bnl/star-sw/pull/152*issuecomment-926265987__;Iw!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8KesU5Va6g$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AUK2XR477RIN7F6VHU3DFITUDPD2FANCNFSM5EPB7AEQ__;!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8KdxKczd9Q$. Triage notifications on the go with GitHub Mobile for iOShttps://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!KwNVnqRv!VDRz1RRyn3fPqTGhyf_9yL78iGmFmxOjo5EvU1saORFfrxr-Cv2lCuGy9G5G8Kd0xBteSQ$ or Androidhttps://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$.

veprbl commented 2 years ago

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

zlchang commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-926270841, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3JBHXFDI6OATM3F4GLUDPFR5ANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

veprbl commented 2 years ago

Pass

zlchang commented 2 years ago

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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#issuecomment-926276124, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3KOCBHMJUU36TDETCTUDPHMJANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

zlchang commented 2 years ago

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.cxxhttps://github.com/star-bnl/star-sw/pull/152#discussion_r715306747:

+

  • 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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#pullrequestreview-762679509, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3IFPQLDA7CFKFEYRV3UDP4J3ANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

zlchang commented 2 years ago

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.cxxhttps://github.com/star-bnl/star-sw/pull/152#discussion_r715306747:

+

  • 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 GitHubhttps://github.com/star-bnl/star-sw/pull/152#pullrequestreview-762679509, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3IFPQLDA7CFKFEYRV3UDP4J3ANCNFSM5EPB7AEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

zlchang commented 2 years ago

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.

veprbl commented 2 years ago

You also need to modify stjtracklist

I don't think we need to have narrow type for stjtrack, it's not written to disk.

veprbl commented 2 years ago

Keeping int for stjettrack should be also fine.