Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FST raw hits in StEvent and StMuDst #378

Merged
merged 37 commits into from
Nov 17, 2022
Merged

Add FST raw hits in StEvent and StMuDst #378

merged 37 commits into from
Nov 17, 2022

Conversation

techuan-huang
Copy link
Contributor

@techuan-huang techuan-huang commented Jul 18, 2022

FST group would like to have FST raw hits information for the detector QA in future calibration productions. So we would like to add this information into StEvent and StMuDst.
In StEvent part, StFstRawHits.{h,cxx} are moved from StFstUtil to StEvent. A new collection (StFstEvtCollection) is created to keep the backward compatibility.
In StMuDst part, 8 files were modified (StMuArrays.cxx/h, StMuDstMaker.cxx, StMuFstCollection.cxx/h, StMuFstUtil.cxx/h, and StMuTypes.hh) and 2 files were added (StMuFstRawHit.cxx/h).
Two chain options (fstEvtRawHit and fstMuRawHit) are added to turn this feature on and off.

@starsdong
Copy link
Member

Hi Techuan, Thank you. I wonder whether you have a comparison on the MuDst file size difference between before and after this change? Thanks

@techuan-huang
Copy link
Contributor Author

techuan-huang commented Jul 18, 2022

Hi Techuan, Thank you. I wonder whether you have a comparison on the MuDst file size difference between before and after this change? Thanks

Hi Xin, I think you pointed out the key.
I tried with a DAQ file with 7907 events and with only FST and sTGC information.
The size before and after were ~28M and ~41M. Its ~48% larger.

@plexoos
Copy link
Member

plexoos commented Jul 21, 2022

FST group would like to have FST raw hits information for the detector QA in future calibration productions.

Sounds like the inclusion of raw hits is required only for special processing, if so this PR should include a new BFC option to turn this feature on when needed but it should be off by default.

@starsdong
Copy link
Member

Hi Te-chuan, it is possible that you can incorporate the suggestion from Dmitri to include a chain option for turning on the FstRawHits in the production?

@techuan-huang
Copy link
Contributor Author

Hi Xin and Dmitri,
Thanks a lot for your suggestions.
I have added a new chain option "fstMuRawHit", and now FST raw hits are NOT stored in MuDst by default.
They will be stored only when the new option is called during the production.

@starsdong
Copy link
Member

Thank you, Techuan. I would like to hear the comments/approval from Daniel B., particularly for the MuDst related changes. Other owners, please feel free to comment as well.

StRoot/StBFChain/BigFullChain.h Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuDstMaker.cxx Outdated Show resolved Hide resolved
@jdbrice
Copy link
Contributor

jdbrice commented Aug 11, 2022

We discussed this in our FWD software meeting. We do not normally add info directly to StMuDst that does not exists in StEvent. We had some discussion about the problems this might cause depending on our future plans with embedding. Because of these I am not ready to approve this, though we may in the future, especially if we reach parity with StEvent first.

@techuan-huang techuan-huang changed the title Add FST raw hits in StMuDst Add FST raw hits in StEvent and StMuDst Aug 30, 2022
@techuan-huang
Copy link
Contributor Author

I have implemented FST raw hits also in StEvent. In order to keep the backward compatibility, I created StFstEvtCollection in StEvent to store the raw hits instead of moving the original StFstCollection to StEvent. A chain option (fstEvtRawHit) similar to the one for StMuDst is also added.

Copy link
Contributor

@klendathu2k klendathu2k left a comment

Choose a reason for hiding this comment

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

Looks like all of my concerns have been addressed.

StRoot/StFstRawHitMaker/StFstRawHitMaker.cxx Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuFstRawHit.cxx Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuFstRawHit.cxx Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuFstRawHit.h Outdated Show resolved Hide resolved
StRoot/StEvent/StFstRawHit.h Outdated Show resolved Hide resolved
StRoot/StFstRawHitMaker/StFstRawHitMaker.cxx Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuFstRawHit.h Outdated Show resolved Hide resolved
@starsdong
Copy link
Member

Daniel, could you please review this PR, comment or approve it so we can proceed soon? Thanks

StRoot/StEvent/StFstRawHit.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StFstRawHit.cxx Outdated Show resolved Hide resolved
StRoot/StEvent/StFstEvtCollection.cxx Outdated Show resolved Hide resolved
StRoot/StMuDSTMaker/COMMON/StMuFstRawHit.cxx Outdated Show resolved Hide resolved
StRoot/StFstRawHitMaker/StFstRawHitMaker.cxx Show resolved Hide resolved
@jdbrice jdbrice merged commit f38e654 into star-bnl:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants