-
Notifications
You must be signed in to change notification settings - Fork 62
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
RHICf Run and Event number added in StRHICfCollection #544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is clear and unobjectionable (save for constness which Dmitri has noted). What is not clear is the rationale. Why do you need to propagate a run and event number, when these are already provided by StEvent? Two possible reasons come to mind...
-
Downstream analysis software expects them to be part of the StEvent / StMuDst and/or pico dst objects. If this is the case, then we should not approve this pull request.
-
The RHICf detector has encoded a run and event numbering scheme that differs from the STAR scheme. If this is the case, why do we need to have two different run/event numbering schemes in our event data model? This is guaranteed to cause confusion in the future.
To expedite, I will try to summarize my understanding on behalf of the RHICf folks: #2 is correct, and this is because RHICf did take their own raw data outside of the STAR DAQ system. That raw data has its own runs and events. By including the RHICf run and event numbers in addition to the STAR run and event numbers that are already accessible in the DSTs, the RHICf group can correlate between findings they made through their own raw data investigations with what they get through the STAR framework. |
Thanks Gene. I thought that would be the case. Inclusion in the STAR event data model is reasonable, But it absolutely must be documented in the header file. |
Dear Jason, and Gene, |
Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>
Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>
Co-authored-by: Gene Van Buren <85305093+genevb@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -50,6 +52,8 @@ const std::vector<StRHICfPoint*>& StRHICfCollection::pointCollection() const {re | |||
void StRHICfCollection::isAllSave(){mRHICfHitColl = new StRHICfHit();} | |||
|
|||
//=========== Set ===========// | |||
void StRHICfCollection::setRHICfRunNumber(UInt_t run){mRHICfRunNumber = run;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important that we not confuse people looking at the data in the future. Please provide a comment block for the run and event number setters, getters and member variables. These comment blocks should clearly state what is being stored/retrieved, and why it differs from the STAR run and event numbering scheme.
PR 1. StRHICfCollection for RHICf Run and Event Number
I will request 4 PRs for adding the RHICf run and event number, before the new STAR release
I believe reviewing these PRs is very simple and clear.
Please review and approve this
Thank you,
Seunghwan