-
Notifications
You must be signed in to change notification settings - Fork 6
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
TPM HDF5 Usage #268
TPM HDF5 Usage #268
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. I would add an extra sanity check, however, as above in the review.
plugins/TriggerPrimitiveMaker.cpp
Outdated
// If we crossed a time boundary, push the current TPSet and reset it | ||
if (current_tpset_number > prev_tpset_number) { | ||
tpset.start_time = prev_tpset_number * m_conf.tpset_time_width + m_conf.tpset_time_offset; | ||
tpset.end_time = tpset.start_time + m_conf.tpset_time_width; |
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.
Is this how this is set in the readout? Does the config have a default value that's the same as readout's default value for TPSet creation?
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.
I did not refer to how TPSets are created in readout
. The changes here were to make use of reading from HDF5 files on the legacy code. There were no logic or definition changes otherwise.
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.
In that case this might not be the final PR for HDF5 reading. I think as much as possible we want to emulate how the readout creates TPSets, so that the replay app is really trying to replay identical conditions.
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.
I agree. There will likely be more changes necessary to catch up to readout
.
For formatting source code files, I suggest using |
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, thank you for all these changes!
TriggerPrimitiveMaker currently uses a txt file to read TriggerPrimitives from. This change allows the TPM to use HDF5 files to read from with the requirement that the HDF5 file contains TimeSlices (implying the file is a tpstream).
This was tested using
generate_tpset_from_tpm.cxx
. This application focuses on the changes made to the TPM by copying the new code and running against an arbitrary tpstream file. This does not test the use of TPM in the replay application.