star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Adding dN/dx parameters for BES-II #34

Closed nigmatkulov closed 2 years ago

plexoos commented 2 years ago

Grigory, should we expect more changes in this PR?

starsdong commented 2 years ago

Rongrong, it seems that this PR needs your review/approval before getting merged. Please let us know whether you are OK.

marrbnl commented 2 years ago

Sorry, I did not realize that that needed my approval.

Two questions:

starsdong commented 2 years ago

Dmitri and Rongrong, I should let Grigory comment more here about the purpose of adding mStatus. I remember he brought this up before suggested this change and the logic I see in the picoDst code does the implementation correctly. gTrack->primaryTrack() is the MuDst function, it will return True for all primary tracks (associated with any primary vertices).

plexoos commented 2 years ago

All I am saying is that to me it looks as if the following piece of code would fill primary momentum of Pico tracks for all MuDst primary tracks. I don't see where the redefinition of primary Pico track happens based on the saved pico vertex. Maybe it happens somewhere else in the code. Perhaps Grigory saw the need for the new flag but it is not very obvious without an explanation

https://github.com/star-bnl/star-sw/blob/c71b0a035d7b3b31cad4191fe54c7acac1b7e8ac/StRoot/StPicoDstMaker/StPicoDstMaker.cxx#L1227-L1233

nigmatkulov commented 2 years ago

@marrbnl:

Do I understand it correctly that now DnDx values and errors are declared if "TFG__VERSION" is not defined? Shouldn't it be the other way around?

Nope. Basically it activates dNdx fields (members) but in case it is not activated it will use 0 bytes.

What is the purpose of adding "picoTrk->setStatus(1)" since only one primary vertex is stored?

Xin is correct. It is needed to address two items: 1) have a hint on the pile-up; 2) minimize number of iterations when searching for the decay particles.

@plexoos

All I am saying is that to me it looks as if the following piece of code would fill primary momentum of Pico tracks for all MuDst primary tracks. I don't see where the redefinition of primary Pico track happens based on the saved pico vertex. Maybe it happens somewhere else in the code. Perhaps Grigory saw the need for the new flag but it is not very obvious without an explanation

I do not understand this question. The pMom keeps momentum of primary tracks from the ONLY ONE vertex. The best one (whatever it means). The status keeps a flag if the track was fitted to any vertex, which is not stored.

plexoos commented 2 years ago

The pMom keeps momentum of primary tracks from the ONLY ONE vertex. The best one (whatever it means).

Can you please point me to the code where this selection is done? It may be that I am just missing it...

There should be a place in the code where pMom for picodst tracks is set to 0 (or remains 0 as default value) for primary mudst tracks not pointing to the best vertex.

genevb commented 2 years ago

We need this in SL21c if we're to produce BES-II data with SL21c, correct?

Thanks, -Gene

genevb commented 2 years ago

On Jul 20, 2021, at 11:44 AM, Gene Van Buren @.***> wrote: We need this in SL21c if we're to produce BES-II data with SL21c, correct?

Or maybe not since we're not including dN/dx in the production? I'd appreciate if someone could clarify this for me. Thanks, -Gene

starsdong commented 2 years ago

Yes, Gene. This is not immediately for the upcoming 19.6 GeV production. However, I am wondering if the library is not built yet, it is probably better to include this commit in to avoid many branches. This way at least we have the functionalities there for R&D developers to look. I think in the production when these variables are all zeros, ROOT should be able to handle the suppression so the size increase should be negligible.

nigmatkulov commented 2 years ago

@genevb we should include it because many strange particle (including hypernuclei) calculations will be affected. It will also help for the future small system analyses where the pile-up presents.

@plexoos There should be a place in the code where pMom for picodst tracks is set to 0 (or remains 0 as default value).

https://github.com/star-bnl/star-sw/blob/c71b0a035d7b3b31cad4191fe54c7acac1b7e8ac/StRoot/StPicoDstMaker/StPicoDstMaker.cxx#L1111

And the default constructor sets it to 0. At the beginning of the function StPicoDstMaker::fillTracks() we have already defined a map between global tracks and those which belong to the best primary vertex.

marrbnl commented 2 years ago

I have approved the pull request. Please let me know if anything else is needed from my side.

Best Rongrong

On Jul 20, 2021, at 12:27 PM, nigmatkulov @.***> wrote:

@genevb https://github.com/genevb we should include it because many strange particle (including hypernuclei) calculations will be affected.

@plexoos https://github.com/plexoos There should be a place in the code where pMom for picodst tracks is set to 0 (or remains 0 as default value).

https://github.com/star-bnl/star-sw/blob/c71b0a035d7b3b31cad4191fe54c7acac1b7e8ac/StRoot/StPicoDstMaker/StPicoDstMaker.cxx#L1111 https://github.com/star-bnl/star-sw/blob/c71b0a035d7b3b31cad4191fe54c7acac1b7e8ac/StRoot/StPicoDstMaker/StPicoDstMaker.cxx#L1111 And the default constructor sets it to 0.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/star-bnl/star-sw/pull/34#issuecomment-883528175, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYQ7XOLEM3JJLSJZ62GO2DTYWPWHANCNFSM462HGWTA.

nigmatkulov commented 2 years ago

@marrbnl, to approve the PR we need at least two persons.

plexoos commented 2 years ago

At the beginning of the function StPicoDstMaker::fillTracks() we have already defined a map between global tracks and those which belong to the best primary vertex.

Ok, now I recall that StMuDst::primaryTracks(int) returns a track based on the currently "selected" vertex rather than all primary tracks... Fine, it makes then.

genevb commented 2 years ago

Dmitri, would you please clarify what hash we use to tag another version of SL21c that includes this update but not all the ROOT6 updates?

Thanks, -Gene

On Jul 20, 2021, at 12:55 PM, nigmatkulov @.**@.>> wrote:

Merged #34https://github.com/star-bnl/star-sw/pull/34 into main.

plexoos commented 2 years ago

Hi Gene, We just branch from 0d53a1a98fb3cadf755744f8d1e08cab089837dc (which is currently tagged as SL21c) and add this commit to the branch. We can then force reset the SL21c tag to the tip of the new branch

plexoos commented 2 years ago

Yesterday, I updated the SL21c tag to include this change in PicoDst but the libraries in /afs do not include it as I confirmed just now. Apparently, Amol built the release before the update so, I asked him to rebuild using the same tag.