Bco counting refactor #7
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (13)
Tip You can disable the changed files summary in the walkthrough.Disable the ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @offline/framework/fun4allraw/SingleGl1PoolInput.h:
- Line 38: The file declares m_total_event initialized with
std::numeric_limits<int>::max() but does not include the <limits> header; add an
#include <limits> near the top of SingleGl1PoolInput.h (before the declaration
of m_total_event) so that std::numeric_limits is defined and the initialization
of m_total_event compiles correctly.
🧹 Nitpick comments (2)
offline/framework/fun4allraw/SingleGl1PoolInput.h (2)
30-31: Consider aligning setter behavior with InputManager.These setters use direct assignment, while
Fun4AllStreamingLumiCountingInputManager::SetNegativeWindowandSetPositiveWindowusestd::max()to ensure the window is never reduced. If multiple components configure these windows, the direct assignment here may inadvertently shrink the window, breaking synchronization.🔎 Proposed alignment with InputManager behavior
- void SetNegativeWindow(const unsigned int value) { m_negative_bco_window = value; } - void SetPositiveWindow(const unsigned int value) { m_positive_bco_window = value; } + void SetNegativeWindow(const unsigned int value) { m_negative_bco_window = std::max(value, m_negative_bco_window); } + void SetPositiveWindow(const unsigned int value) { m_positive_bco_window = std::max(value, m_positive_bco_window); }
36-40: Consider documenting BCO window defaults and standardizing initialization.The hardcoded values 20 and 325 for BCO windows lack inline comments explaining their physical or operational meaning. Additionally, the initialization syntax is inconsistent: line 38 uses
=while lines 39-40 use brace initialization{false}(the braces are redundant for simple scalar types).🔎 Suggested improvements
- unsigned int m_negative_bco_window = 20; - unsigned int m_positive_bco_window = 325; + unsigned int m_negative_bco_window = 20; // Default backward BCO window + unsigned int m_positive_bco_window = 325; // Default forward BCO window int m_total_event = std::numeric_limits<int>::max(); - bool m_alldone_flag = {false}; - bool m_lastevent_flag = {false}; + bool m_alldone_flag = false; + bool m_lastevent_flag = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.ccoffline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.hoffline/framework/fun4allraw/Makefile.amoffline/framework/fun4allraw/SingleGl1PoolInput.ccoffline/framework/fun4allraw/SingleGl1PoolInput.hoffline/framework/fun4allraw/SingleStreamingInput.hoffline/framework/rawbcolumi/Makefile.amoffline/framework/rawbcolumi/SingleGl1PoolInputv2.ccoffline/framework/rawbcolumi/SingleGl1PoolInputv2.hoffline/framework/rawbcolumi/SingleStreamingInputv2.ccoffline/framework/rawbcolumi/SingleStreamingInputv2.hoffline/framework/rawbcolumi/autogen.shoffline/framework/rawbcolumi/configure.ac
💤 Files with no reviewable changes (7)
- offline/framework/rawbcolumi/SingleStreamingInputv2.cc
- offline/framework/rawbcolumi/SingleStreamingInputv2.h
- offline/framework/rawbcolumi/Makefile.am
- offline/framework/rawbcolumi/autogen.sh
- offline/framework/rawbcolumi/SingleGl1PoolInputv2.h
- offline/framework/rawbcolumi/configure.ac
- offline/framework/rawbcolumi/SingleGl1PoolInputv2.cc
🧰 Additional context used
🧬 Code graph analysis (4)
offline/framework/fun4allraw/SingleGl1PoolInput.cc (1)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h (2)
m_negative_bco_window(66-66)m_positive_bco_window(67-67)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h (1)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc (2)
registerStreamingInput(176-214)registerStreamingInput(176-176)
offline/framework/fun4allraw/SingleStreamingInput.h (2)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc (2)
Fun4AllStreamingLumiCountingInputManager(38-47)Fun4AllStreamingLumiCountingInputManager(49-66)offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h (1)
Fun4AllStreamingLumiCountingInputManager(20-44)
offline/framework/fun4allraw/SingleGl1PoolInput.h (2)
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc (4)
SetNegativeWindow(502-505)SetNegativeWindow(502-502)SetPositiveWindow(507-510)SetPositiveWindow(507-507)offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h (2)
m_negative_bco_window(66-66)m_positive_bco_window(67-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (12)
offline/framework/fun4allraw/SingleGl1PoolInput.h (1)
44-45: The memory concern does not apply—these maps are properly cleaned up.Both
m_BCOWindowsandm_BCOBunchNumberare explicitly erased inCleanupUsedPackets()at lines 271–272 ofSingleGl1PoolInput.cc. Entries are removed when their BCO values become older than the reference BCO passed to the cleanup method, preventing unbounded growth in long-running processes.Likely an incorrect or invalid review comment.
offline/framework/fun4allraw/SingleGl1PoolInput.cc (6)
4-4: LGTM!The include for the Lumi counting input manager is correctly added to support the new functionality.
73-91: Verify the 5000-event batching logic.The code sets
m_alldone_flagandm_lastevent_flagevery 5000 events (line 79-83), then clears them after processing (lines 201-205). This creates a periodic end-of-event signal that triggers output file writing in the Lumi counting manager (see line 465-497 in Fun4AllStreamingLumiCountingInputManager.cc).Confirm this batching behavior is intentional and that 5000 events is the desired batch size for Lumi counting output.
122-131: LGTM!Proper null pointer check for
StreamingLumiInputManager()before invoking its methods. The handling of a null packet 14001 by setting end-of-event flags is appropriate.
192-200: LGTM!Proper null pointer check before invoking
StreamingLumiInputManager()methods. The propagation of BCO windows, bunch numbers, event flags, and window settings to the Lumi counting manager is correctly implemented.
201-205: LGTM!The flag reset (lines 201-205) complements the periodic flag setting, completing the 5000-event batching logic.
The cleanup logic (lines 270-272) correctly uses the trimmed 40-bit BCO to erase associated Lumi counting state from
m_BCOWindowsandm_BCOBunchNumber, maintaining consistency with the trimming applied at line 140.Also applies to: 270-272
140-143: All flagged concerns are not valid issues in the code.
Member variable declarations: Both
m_BCOWindows(line 44) andm_BCOBunchNumber(line 45) are properly declared in the header file as private members.Window variable initialization:
m_negative_bco_windowandm_positive_bco_windoware not uninitialized—they have default values of 20 and 325 respectively (lines 36–37 in the header). They can be optionally overridden viaSetNegativeWindow()andSetPositiveWindow()setters.40-bit BCO mask: The mask
0xFFFFFFFFFFUis correct and follows established convention throughout the codebase (confirmed in SingleInttPoolInput.cc, Fun4AllStreamingInputManager.cc, and documented in comments as "40 bit clock").Likely an incorrect or invalid review comment.
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc (2)
4-4: LGTM!The include statement correctly updated from the deprecated v2 variant to the base
SingleStreamingInputclass.
176-176: LGTM!The function signature correctly migrated from
SingleStreamingInputv2*toSingleStreamingInput*, consistent with the type refactoring across the codebase.offline/framework/fun4allraw/Makefile.am (1)
21-21: LGTM!Build configuration correctly updated to export the
Fun4AllStreamingLumiCountingInputManagerheader and compile its implementation, making the Lumi counting manager publicly available.Also applies to: 79-79
offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h (1)
14-14: LGTM!Type migration from
SingleStreamingInputv2toSingleStreamingInputis correctly applied across:
- Forward declaration (line 14)
- Method signature (line 37)
- Member variable type (line 79)
The changes are consistent with the refactoring to remove the v2 variant.
Also applies to: 37-37, 79-79
offline/framework/fun4allraw/SingleStreamingInput.h (1)
15-15: LGTM!The API extension cleanly integrates the Lumi counting manager:
- Forward declaration added (line 15)
- Accessor/mutator pair follows the existing pattern used for
StreamingInputManager(lines 49-50)- Private member properly initialized to
nullptr(line 123)The design is consistent and maintains encapsulation.
Also applies to: 51-52, 123-124
|
|
||
| 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 required header for std::numeric_limits.
Line 38 uses std::numeric_limits<int>::max() but the header does not include <limits>, which will cause a compilation error.
🔎 Proposed fix
Add the missing include near the top of the file:
#include <cstdint>
+#include <limits>
#include <list>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @offline/framework/fun4allraw/SingleGl1PoolInput.h at line 38, The file
declares m_total_event initialized with std::numeric_limits<int>::max() but does
not include the <limits> header; add an #include <limits> near the top of
SingleGl1PoolInput.h (before the declaration of m_total_event) so that
std::numeric_limits is defined and the initialization of m_total_event compiles
correctly.
Docstrings generation was requested by @blackcathj. * #7 (comment) The following files were modified: * `offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc` * `offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h` * `offline/framework/fun4allraw/SingleGl1PoolInput.cc` * `offline/framework/fun4allraw/SingleGl1PoolInput.h` * `offline/framework/fun4allraw/SingleStreamingInput.h`
There was a problem hiding this comment.
Pull request overview
This PR refactors the BCO (Beam Clock) counting infrastructure by consolidating functionality from the rawbcolumi directory into the existing fun4allraw directory. The changes eliminate the SingleStreamingInputv2 classes and integrate luminosity counting features directly into the base SingleStreamingInput class.
Key Changes
- Removed the entire
rawbcolumidirectory includingSingleStreamingInputv2andSingleGl1PoolInputv2classes - Extended
SingleStreamingInputbase class with luminosity counting manager support - Enhanced
SingleGl1PoolInputwith BCO window tracking, bunch number management, and event control flags - 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 4 comments.
Show a summary per file
| File | Description |
|---|---|
| offline/framework/rawbcolumi/configure.ac | Deleted autoconf configuration for removed rawbcolumi library |
| offline/framework/rawbcolumi/autogen.sh | Deleted autogen script for removed rawbcolumi library |
| offline/framework/rawbcolumi/SingleStreamingInputv2.h | Deleted v2 header, functionality migrated to base class |
| offline/framework/rawbcolumi/SingleStreamingInputv2.cc | Deleted v2 implementation, functionality migrated to base class |
| offline/framework/rawbcolumi/SingleGl1PoolInputv2.h | Deleted v2 header, functionality migrated to base class |
| offline/framework/rawbcolumi/SingleGl1PoolInputv2.cc | Deleted v2 implementation, functionality migrated to base class |
| offline/framework/rawbcolumi/Makefile.am | Deleted build configuration for removed rawbcolumi library |
| offline/framework/fun4allraw/SingleStreamingInput.h | Added lumi counting manager pointer and accessor methods |
| offline/framework/fun4allraw/SingleGl1PoolInput.h | Added BCO window controls, bunch number tracking, and event control flags |
| offline/framework/fun4allraw/SingleGl1PoolInput.cc | Implemented BCO window calculation, bunch number tracking, and integration with lumi counting manager |
| offline/framework/fun4allraw/Makefile.am | Added Fun4AllStreamingLumiCountingInputManager to build |
| offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.h | Changed to use SingleStreamingInput base class instead of v2 |
| offline/framework/fun4allraw/Fun4AllStreamingLumiCountingInputManager.cc | Updated registration to use SingleStreamingInput base class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::cout << PHWHERE << "Fetching next Event" << evt->getEvtSequence() << std::endl; | ||
| } | ||
| if ((m_total_event == 0 && evt->getEvtType() == ENDRUNEVENT) || | ||
| (m_total_event != 0 && evt->getEvtSequence() - 2 == m_total_event)) |
There was a problem hiding this comment.
Potential integer underflow if evt->getEvtSequence() returns 0 or 1. The expression evt->getEvtSequence() - 2 could underflow since getEvtSequence() likely returns an unsigned type. Consider checking if the value is >= 2 before performing the subtraction, or explicitly cast to a signed type if negative values are intended.
| (m_total_event != 0 && evt->getEvtSequence() - 2 == m_total_event)) | |
| (m_total_event != 0 && evt->getEvtSequence() >= 2 && | |
| evt->getEvtSequence() - 2 == m_total_event)) |
|
|
||
| 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. Line 38 uses std::numeric_limits::max() but the header does not include . This will cause a compilation error. Add #include <limits> to the header file.
| if (evt->getEvtSequence() % 5000 == 0) | ||
| { | ||
| m_alldone_flag = true; | ||
| m_lastevent_flag = true; | ||
| } |
There was a problem hiding this comment.
Confusing logic that sets flags every 5000 events regardless of other conditions. Lines 79-83 set m_alldone_flag and m_lastevent_flag to true whenever the event sequence is a multiple of 5000, but then lines 201-205 immediately reset these flags back to false for the same condition. This creates inconsistent behavior where the flags are briefly set to true but then reset, which may not achieve the intended behavior. Consider clarifying the logic or removing this if-statement.
| if (evt->getEvtSequence() % 5000 == 0) | |
| { | |
| m_alldone_flag = true; | |
| m_lastevent_flag = true; | |
| } |
| 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 unsigned integer underflow. When bco_trim is less than m_negative_bco_window (default 20), the subtraction will wrap around to a very large uint64_t value. While this might be intentional behavior for handling BCO wraparound scenarios, it could lead to unexpected results. Consider adding a check or a comment explaining this is intentional if wraparound behavior is expected.
| m_BCOWindows[bco_trim] = std::make_pair(bco_trim - m_negative_bco_window, bco_trim + m_positive_bco_window); | |
| uint64_t window_start = 0; | |
| if (bco_trim >= static_cast<uint64_t>(m_negative_bco_window)) | |
| { | |
| window_start = bco_trim - static_cast<uint64_t>(m_negative_bco_window); | |
| } | |
| m_BCOWindows[bco_trim] = std::make_pair(window_start, bco_trim + m_positive_bco_window); |
2 participants
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)
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.