refactor: bco counting code #3996
Conversation
|
Let's wait until next week to merge this in case there are unforeseen consequences... |
Build & test reportReport for commit 45a96b5c0b1358f0812a401182b9b18db0f09c57:
Automatically generated by sPHENIX Jenkins continuous integration |
|
@pinkenburg this shouldn't break anything as it is just adding additional functionality - but a second set of eyes wouldn't hurt... |
There was a problem hiding this comment.
Pull request overview
This PR refactors the BCO (Beam Clock) luminosity counting code by consolidating the standalone rawbcolumi v2 implementations into the main fun4allraw pool input framework. This improves maintainability by eliminating code duplication and ensuring that luminosity counting benefits from ongoing framework improvements.
Key Changes
- Removed the entire
offline/framework/rawbcolumi/directory containing standalone v2 implementations - Integrated luminosity counting functionality directly into
SingleStreamingInputandSingleGl1PoolInputclasses - Updated
Fun4AllStreamingLumiCountingInputManagerto work with the baseSingleStreamingInputclass instead of the v2 variant
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| offline/framework/rawbcolumi/* | Removed entire standalone rawbcolumi directory (configure.ac, autogen.sh, Makefile.am, and all v2 implementation files) |
| offline/framework/fun4allraw/SingleStreamingInput.h | Added luminosity counting manager pointer and accessor methods to base streaming input class |
| offline/framework/fun4allraw/SingleGl1PoolInput.h | Added BCO window tracking member variables and setter methods for luminosity counting |
| offline/framework/fun4allraw/SingleGl1PoolInput.cc | Integrated luminosity counting logic including window calculations, event flags, and manager interactions |
| offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h | Updated to use base SingleStreamingInput class instead of v2 variant |
| offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc | Updated include to use base class |
| offline/framework/fun4allraw/Makefile.am | Added Fun4AllStreamingLumiCountingInputManager to build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (evt->getEvtSequence() % 5000 == 0) | ||
| { | ||
| m_alldone_flag = true; | ||
| m_lastevent_flag = true; | ||
| } |
There was a problem hiding this comment.
The logic here sets the done flags every 5000 events, but this condition will be true multiple times (events 5000, 10000, 15000, etc.). The condition should likely use modulo with a specific check or a different approach. If the intent is to only set flags at specific event intervals, consider using a more precise condition or documenting why flags are set repeatedly.
| if (evt->getEvtSequence() % 5000 == 0) | |
| { | |
| m_alldone_flag = true; | |
| m_lastevent_flag = true; | |
| } | |
| // Removed periodic setting of done flags every 5000 events as it is likely incorrect. |
| Gl1Packet *newhit = new Gl1Packetv3(); | ||
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | ||
| uint64_t bco_trim = gtm_bco & 0xFFFFFFFFFFU; | ||
| m_BCOWindows[bco_trim] = std::make_pair(bco_trim - m_negative_bco_window, bco_trim + m_positive_bco_window); |
There was a problem hiding this comment.
Potential underflow issue: subtracting m_negative_bco_window (default 20) from bco_trim could underflow if bco_trim is less than 20, since both are unsigned integers. This could result in a very large value wrapping around. Consider adding a check or casting to signed integers before subtraction, or ensuring bco_trim is always larger than m_negative_bco_window.
| m_BCOWindows[bco_trim] = std::make_pair(bco_trim - m_negative_bco_window, bco_trim + m_positive_bco_window); | |
| uint64_t bco_window_lower = (bco_trim >= m_negative_bco_window) ? (bco_trim - m_negative_bco_window) : 0; | |
| m_BCOWindows[bco_trim] = std::make_pair(bco_window_lower, bco_trim + m_positive_bco_window); |
| if (evt->getEvtSequence() % 5000 == 0) | ||
| { | ||
| m_alldone_flag = false; | ||
| m_lastevent_flag = false; | ||
| } |
There was a problem hiding this comment.
The flags are reset here after being potentially set on lines 79-83, but only for events at multiples of 5000. This creates inconsistent behavior where the flags are set at line 79-83 for all events divisible by 5000, then immediately reset at line 201-205 for the same events. The logic for setting and resetting these flags should be clarified and potentially consolidated to avoid this confusing pattern.
| if (evt->getEvtSequence() % 5000 == 0) | |
| { | |
| m_alldone_flag = false; | |
| m_lastevent_flag = false; | |
| } | |
| // Removed resetting of m_alldone_flag and m_lastevent_flag here to avoid inconsistent behavior. | |
| // if (evt->getEvtSequence() % 5000 == 0) | |
| // { | |
| // m_alldone_flag = false; | |
| // m_lastevent_flag = false; | |
| // } |
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | ||
| uint64_t bco_trim = gtm_bco & 0xFFFFFFFFFFU; | ||
| m_BCOWindows[bco_trim] = std::make_pair(bco_trim - m_negative_bco_window, bco_trim + m_positive_bco_window); | ||
| // std::cout<<"BCO "<< m_BCOWindows.begin()->first<<" left "<<m_BCOWindows.begin()->second.first<<" right "<< m_BCOWindows.begin()->second.second<<std::endl; |
There was a problem hiding this comment.
Commented-out debug code should be removed rather than left in the production codebase. If this output is needed for debugging, consider using the Verbosity() mechanism already in place throughout this file.
| // std::cout<<"BCO "<< m_BCOWindows.begin()->first<<" left "<<m_BCOWindows.begin()->second.first<<" right "<< m_BCOWindows.begin()->second.second<<std::endl; | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "BCO " << bco_trim | |
| << " left " << m_BCOWindows[bco_trim].first | |
| << " right " << m_BCOWindows[bco_trim].second | |
| << std::endl; | |
| } |
| { | ||
| std::cout << "gl1 all done is true" << std::endl; | ||
| } | ||
| // else{std::cout<<"gl1 all done is false"<<std::endl;} |
There was a problem hiding this comment.
Commented-out debug code should be removed. If this alternative branch output is needed, consider uncommenting it or removing it entirely to keep the codebase clean.
| // else{std::cout<<"gl1 all done is false"<<std::endl;} |
| bool m_alldone_flag = {false}; | ||
| bool m_lastevent_flag = {false}; |
There was a problem hiding this comment.
The member variable names m_alldone_flag and m_lastevent_flag use snake_case, which is inconsistent with the camelCase convention used for other member variables in this class (e.g., m_NumSpecialEvents, m_BcoRange). Consider renaming to m_AllDoneFlag and m_LastEventFlag for consistency.
| bool m_alldone_flag = {false}; | |
| bool m_lastevent_flag = {false}; | |
| bool m_AllDoneFlag = {false}; | |
| bool m_LastEventFlag = {false}; |
| unsigned int m_negative_bco_window = 20; | ||
| unsigned int m_positive_bco_window = 325; |
There was a problem hiding this comment.
The member variable names m_negative_bco_window and m_positive_bco_window use snake_case, which is inconsistent with the camelCase convention used for other member variables in this class (e.g., m_NumSpecialEvents, m_BcoRange). Consider renaming to m_NegativeBcoWindow and m_PositiveBcoWindow for consistency.
|
|
||
| unsigned int m_negative_bco_window = 20; | ||
| unsigned int m_positive_bco_window = 325; | ||
| int m_total_event = std::numeric_limits<int>::max(); |
There was a problem hiding this comment.
The member variable name m_total_event uses snake_case, which is inconsistent with the camelCase convention used for other member variables in this class (e.g., m_NumSpecialEvents, m_BcoRange). Consider renaming to m_TotalEvent for consistency.
|
|
||
| unsigned int m_negative_bco_window = 20; | ||
| unsigned int m_positive_bco_window = 325; | ||
| int m_total_event = std::numeric_limits<int>::max(); |
There was a problem hiding this comment.
Missing include for std::numeric_limits. The header uses std::numeric_limits::max() but does not include . This will cause a compilation error. Add #include to the includes section.
1 participant
This PR refactors the bcolumi code added by Zhiwan into the main pool input objects so that maintenance long term is easier. Currently the v2 objects that were added are standalone, and thus, don't benefit from any of the changes in the last months. This should not change any default performance/functionality.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)