Streaming Lumi Counting and BCO Tagging#4269
Conversation
…rom BcoLumiReco output that contains the gl1 bco, the corresponding streaming window, a boolean to encode if the gl1 bco is usable for analysis, and the luminosity in the streaming data. (2) Add corresponding PHObject classes for storing the luminosity and gl1 info in the DST. (3) Add Subsys reco module to test new DST output.
📝 WalkthroughWalkthroughThis PR replaces the legacy ChangesStreaming BCO/Lumi Pipeline Implementation
Possibly Related PRs
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 |
Build & test reportReport for commit bdf2ea497eb26d18012f3bccd6285d9223b49133:
Automatically generated by sPHENIX Jenkins continuous integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d69f6fc4-69a4-44f7-9043-a8a2e301e73e
📒 Files selected for processing (19)
offline/packages/bcolumicount/BcoLumiCheck.ccoffline/packages/bcolumicount/BcoLumiCheck.hoffline/packages/bcolumicount/Makefile.amoffline/packages/bcolumicount/StreamingBcoInfo.ccoffline/packages/bcolumicount/StreamingBcoInfo.hoffline/packages/bcolumicount/StreamingBcoInfoLinkDef.hoffline/packages/bcolumicount/StreamingBcoInfov1.ccoffline/packages/bcolumicount/StreamingBcoInfov1.hoffline/packages/bcolumicount/StreamingBcoInfov1LinkDef.hoffline/packages/bcolumicount/StreamingBcoLumiCheck.ccoffline/packages/bcolumicount/StreamingBcoLumiCheck.hoffline/packages/bcolumicount/StreamingBcoLumiReco.ccoffline/packages/bcolumicount/StreamingBcoLumiReco.hoffline/packages/bcolumicount/StreamingLumiInfo.ccoffline/packages/bcolumicount/StreamingLumiInfo.hoffline/packages/bcolumicount/StreamingLumiInfoLinkDef.hoffline/packages/bcolumicount/StreamingLumiInfov1.ccoffline/packages/bcolumicount/StreamingLumiInfov1.hoffline/packages/bcolumicount/StreamingLumiInfov1LinkDef.h
💤 Files with no reviewable changes (2)
- offline/packages/bcolumicount/BcoLumiCheck.h
- offline/packages/bcolumicount/BcoLumiCheck.cc
| void StreamingBcoInfov1::Reset() | ||
| { | ||
| set_bco(0); | ||
| set_usable_bco_tag(false); | ||
| set_bco_streaming_window(std::make_pair(0, 0)); | ||
| return; |
There was a problem hiding this comment.
Reset leaves evtno stale across object reuse.
StreamingBcoInfov1::Reset() does not clear the event number, so previous-event metadata can persist after reset and propagate wrong DST bookkeeping.
Proposed fix
void StreamingBcoInfov1::Reset()
{
set_bco(0);
+ set_evtno(0);
set_usable_bco_tag(false);
set_bco_streaming_window(std::make_pair(0, 0));
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void StreamingBcoInfov1::Reset() | |
| { | |
| set_bco(0); | |
| set_usable_bco_tag(false); | |
| set_bco_streaming_window(std::make_pair(0, 0)); | |
| return; | |
| void StreamingBcoInfov1::Reset() | |
| { | |
| set_bco(0); | |
| set_evtno(0); | |
| set_usable_bco_tag(false); | |
| set_bco_streaming_window(std::make_pair(0, 0)); | |
| return; | |
| } |
There was a problem hiding this comment.
This is a relevant comment from the rabbit, we should reset all member variables of the EDM object. I think. Or perhaps the idea is to only set the event number once and have it be incremented on each process_event call? We may consider seeing if there is a "global" event number owned by fun4all that could be set here each reset call, so that it is guaranteed that this object is always in sync with Fun4all
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how evtno is set in StreamingBcoLumiReco process_event
rg -n "set_evtno\|get_evtno\|evtno\|EventNumber\|getEventNumber" --type=cc -C3
# Check Fun4AllServer for global event number accessor
rg -n "getEventNumber\|EventCounter\|m_eventcount\|m_eventcounter" --type=cc -C2
# Look at other PHObject v1 implementations for how Reset() handles evtno-like fields
rg -n "set_evtno\|m_evtno" -C2
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check how evtno is set in StreamingBcoLumiReco process_event
rg -n "set_evtno\|get_evtno\|m_evtno" -g "*.cc" -g "*.h" -C3
# Check Fun4AllServer for a global event number accessor
rg -n "getEventNumber\|EventCounter\|m_eventcount" -g "*.cc" -g "*.h" -C2
# Check how SyncObject or other mechanisms track event numbers in this package
rg -n "evtno\|evt_number\|event_number\|EventNumber" -g "*.cc" -g "*.h" --iglob "*bcolumicount*" -C3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all files in the bcolumicount package
fd . offline/packages/bcolumicount/
# Cat the streaming bco lumi reco implementation
cat offline/packages/bcolumicount/StreamingBcoLumiReco.cc
# Cat the v1 header to see all private members
cat offline/packages/bcolumicount/StreamingBcoInfov1.h
Length of output: 12683
@osbornjd — good question. Looking at StreamingBcoLumiReco::process_event(), the answer is actually both problems at once: m_evtno is never written anywhere — there is no streaming_bco_info->set_evtno(...) call in process_event(). So it is both stale on object reuse and never populated in the first place.
On the Fun4All global event number question: you're already fetching the right handle. The SyncObject is retrieved at the top of process_event() and syncobject->EventNumber() is used in the debug printout. That is the canonical per-event counter in Fun4All and is exactly what should be written to StreamingBcoInfov1. No need to reach for Fun4AllServer directly.
The fix then has two parts:
1. StreamingBcoInfov1::Reset() — zero all members:
void StreamingBcoInfov1::Reset()
{
set_bco(0);
set_evtno(0); // add this
set_usable_bco_tag(false);
set_bco_streaming_window(std::make_pair(0, 0));
}2. StreamingBcoLumiReco::process_event() — populate evtno from SyncObject:
if (bcoinfo)
{
// ... existing BCO logic ...
streaming_bco_info->set_bco(get_bco());
streaming_bco_info->set_usable_bco_tag(get_usable_bco_tag());
streaming_bco_info->set_bco_streaming_window(get_bco_streaming_window());
if (syncobject) // add null guard
{
streaming_bco_info->set_evtno(syncobject->EventNumber()); // add this
}
}Note the null guard on syncobject — it is currently dereferenced for debug output without a null check, which is an independent correctness issue worth fixing at the same time.
🧠 Learnings used
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4268
File: offline/packages/CaloReco/ClusterCDFCalculator.cc:9-9
Timestamp: 2026-05-18T16:21:14.018Z
Learning: In the sPHENIX core software repository, C++20 std::format is supported (via the <format> header). During code review, do not flag uses of std::format / <format> as a potential C++-version or compiler-compatibility concern anywhere in the C++ codebase. (Other issues may still be flagged as appropriate.)
| SyncObject *syncobject = findNode::getClass<SyncObject>(topNode, syncdefs::SYNCNODENAME); | ||
| //Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001); | ||
| Event *evt = findNode::getClass<Event>(topNode, "PRDF"); | ||
| if (evt) | ||
| { | ||
| if (Verbosity() > 2) | ||
| { | ||
| evt->identify(); | ||
| } | ||
| if (evt->getEvtType() != DATAEVENT) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| Packet *packet = evt->getPacket(14001); | ||
| if (!packet) | ||
| { | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "no gl1 packet 14001" << std::endl; | ||
| evt->identify(); | ||
| } | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | ||
| uint64_t gl1_scaledvec = packet->lValue(0, "ScaledVector"); | ||
| //uint64_t gl1_livevec = packet->lValue(0, "TriggerVector"); | ||
|
|
||
| int bunchno = packet->lValue(0,"BunchNumber"); | ||
|
|
||
| // SYNTAX TAKEN FROM ZHIWANS CODE, why = and not +=? If this is correct it seems like a waste to call it for every event (would just need it for the last event in a particular crossing?) | ||
| m_bunchnumber_MBDNS_raw[bunchno] = packet->lValue(0, "GL1PRAW"); | ||
| m_bunchnumber_MBDNS_live[bunchno] = packet->lValue(0, "GL1PLIVE"); | ||
| m_bunchnumber_MBDNS_scaled[bunchno] = packet->lValue(0, "GL1PSCALED"); | ||
|
|
||
|
|
||
| if(packet->lValue(0, 0)) | ||
| { | ||
| m_rawgl1scaler = packet->lValue(0, 0); | ||
| } | ||
|
|
||
| delete packet; | ||
|
|
||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "Event No: " << syncobject->EventNumber() /*<< std::hex*/ | ||
| << " gl1 bco: " << gtm_bco <<std::dec << std::endl; | ||
| } |
There was a problem hiding this comment.
Add null guards before dereferencing framework nodes.
syncobject (Line 147) and streaming_bco_info (Line 237 onward) are dereferenced without null checks. Missing nodes here become hard crashes.
Proposed fix
BcoInfo *bcoinfo = findNode::getClass<BcoInfo>(topNode, "BCOINFO");
SyncObject *syncobject = findNode::getClass<SyncObject>(topNode, syncdefs::SYNCNODENAME);
@@
if (Verbosity() > 2)
{
- std::cout << "Event No: " << syncobject->EventNumber() /*<< std::hex*/
+ if (!syncobject)
+ {
+ std::cout << PHWHERE << " SyncObject missing" << std::endl;
+ delete packet;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
+ std::cout << "Event No: " << syncobject->EventNumber() /*<< std::hex*/
<< " gl1 bco: " << gtm_bco <<std::dec << std::endl;
}
@@
StreamingBcoInfo *streaming_bco_info = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO");
+ if (!streaming_bco_info)
+ {
+ std::cout << PHWHERE << " STREAMINGBCOINFO node missing" << std::endl;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }As per coding guidelines: **/*.{cc,cpp,cxx,c} must prioritize correctness and error handling.
Also applies to: 162-239
| int bunchno = packet->lValue(0,"BunchNumber"); | ||
|
|
||
| // SYNTAX TAKEN FROM ZHIWANS CODE, why = and not +=? If this is correct it seems like a waste to call it for every event (would just need it for the last event in a particular crossing?) | ||
| m_bunchnumber_MBDNS_raw[bunchno] = packet->lValue(0, "GL1PRAW"); | ||
| m_bunchnumber_MBDNS_live[bunchno] = packet->lValue(0, "GL1PLIVE"); | ||
| m_bunchnumber_MBDNS_scaled[bunchno] = packet->lValue(0, "GL1PSCALED"); |
There was a problem hiding this comment.
Validate bunch index before writing per-bunch arrays.
Line 133 through Line 135 index fixed-size arrays with bunchno from packet data without bounds checks. An out-of-range bunch id will write out of bounds and corrupt memory.
Proposed fix
int bunchno = packet->lValue(0,"BunchNumber");
+if (bunchno < 0 || bunchno >= m_bunches)
+{
+ if (Verbosity() > 0)
+ {
+ std::cout << PHWHERE << " invalid bunch number: " << bunchno << std::endl;
+ }
+ delete packet;
+ return Fun4AllReturnCodes::ABORTEVENT;
+}
m_bunchnumber_MBDNS_raw[bunchno] = packet->lValue(0, "GL1PRAW");
m_bunchnumber_MBDNS_live[bunchno] = packet->lValue(0, "GL1PLIVE");
m_bunchnumber_MBDNS_scaled[bunchno] = packet->lValue(0, "GL1PSCALED");As per coding guidelines: **/*.{cc,cpp,cxx,c} must prioritize correctness and memory safety.
| uint64_t rawgl1scalers_per_bunch = m_rawgl1scaler/120.; | ||
| for (int i=0; i<m_bunches; i++) | ||
| { | ||
| m_bunchnumber_lumi_raw[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_raw[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | ||
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | ||
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | ||
|
|
There was a problem hiding this comment.
Guard lumi normalization against zero/invalid denominator.
Line 247 truncates m_rawgl1scaler/120. into uint64_t; small values become zero and Line 250–252 then divide by zero. This can silently produce invalid lumi values.
Proposed fix
-uint64_t rawgl1scalers_per_bunch = m_rawgl1scaler/120.;
+const double rawgl1scalers_per_bunch = static_cast<double>(m_rawgl1scaler) / 120.0;
+if (rawgl1scalers_per_bunch <= 0.0 || m_xsec_MBDNS <= 0.0)
+{
+ std::cout << PHWHERE << " invalid normalization inputs: rawgl1scalers_per_bunch="
+ << rawgl1scalers_per_bunch << " xsec=" << m_xsec_MBDNS << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+}As per coding guidelines: **/*.{cc,cpp,cxx,c} must prioritize correctness and error handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint64_t rawgl1scalers_per_bunch = m_rawgl1scaler/120.; | |
| for (int i=0; i<m_bunches; i++) | |
| { | |
| m_bunchnumber_lumi_raw[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_raw[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | |
| const double rawgl1scalers_per_bunch = static_cast<double>(m_rawgl1scaler) / 120.0; | |
| if (rawgl1scalers_per_bunch <= 0.0 || m_xsec_MBDNS <= 0.0) | |
| { | |
| std::cout << PHWHERE << " invalid normalization inputs: rawgl1scalers_per_bunch=" | |
| << rawgl1scalers_per_bunch << " xsec=" << m_xsec_MBDNS << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| for (int i=0; i<m_bunches; i++) | |
| { | |
| m_bunchnumber_lumi_raw[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_raw[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * m_xsec_MBDNS ); |
| m_lumi_raw += m_bunchnumber_lumi_raw[i]; | ||
| m_lumi_live += m_bunchnumber_lumi_live[i]; | ||
| m_lumi_scaled += m_bunchnumber_lumi_scaled[i]; | ||
|
|
||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << "bunchno : " << i << " lumi_raw : " << m_bunchnumber_lumi_raw[i] << std::endl; | ||
| } | ||
| } | ||
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | ||
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | ||
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << "MBD xsec : " << m_xsec_MBDNS << std::endl; | ||
| std::cout << "total lumi (raw) : " << m_lumi_raw << std::endl; | ||
| } | ||
|
|
||
| m_bunchnumber_lumi_raw.fill(0); | ||
| m_bunchnumber_lumi_live.fill(0); | ||
| m_bunchnumber_lumi_scaled.fill(0); | ||
| m_bunchnumber_crossings.fill(0); | ||
| m_bunchnumber_MBDNS_raw.fill(0); | ||
| m_bunchnumber_MBDNS_live.fill(0); | ||
| m_bunchnumber_MBDNS_scaled.fill(0); | ||
|
|
There was a problem hiding this comment.
Reset run-level accumulators to avoid cross-run contamination.
m_lumi_raw, m_lumi_live, m_lumi_scaled, and m_rawgl1scaler are not reset after EndRun. Next runs accumulate stale totals and can publish incorrect per-run lumi.
Proposed fix
m_bunchnumber_MBDNS_raw.fill(0);
m_bunchnumber_MBDNS_live.fill(0);
m_bunchnumber_MBDNS_scaled.fill(0);
+ m_lumi_raw = 0.;
+ m_lumi_live = 0.;
+ m_lumi_scaled = 0.;
+ m_rawgl1scaler = 0;
return Fun4AllReturnCodes::EVENT_OK;As per coding guidelines: **/*.{cc,cpp,cxx,c} must prioritize correctness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_lumi_raw += m_bunchnumber_lumi_raw[i]; | |
| m_lumi_live += m_bunchnumber_lumi_live[i]; | |
| m_lumi_scaled += m_bunchnumber_lumi_scaled[i]; | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "bunchno : " << i << " lumi_raw : " << m_bunchnumber_lumi_raw[i] << std::endl; | |
| } | |
| } | |
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | |
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | |
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "MBD xsec : " << m_xsec_MBDNS << std::endl; | |
| std::cout << "total lumi (raw) : " << m_lumi_raw << std::endl; | |
| } | |
| m_bunchnumber_lumi_raw.fill(0); | |
| m_bunchnumber_lumi_live.fill(0); | |
| m_bunchnumber_lumi_scaled.fill(0); | |
| m_bunchnumber_crossings.fill(0); | |
| m_bunchnumber_MBDNS_raw.fill(0); | |
| m_bunchnumber_MBDNS_live.fill(0); | |
| m_bunchnumber_MBDNS_scaled.fill(0); | |
| m_lumi_raw += m_bunchnumber_lumi_raw[i]; | |
| m_lumi_live += m_bunchnumber_lumi_live[i]; | |
| m_lumi_scaled += m_bunchnumber_lumi_scaled[i]; | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "bunchno : " << i << " lumi_raw : " << m_bunchnumber_lumi_raw[i] << std::endl; | |
| } | |
| } | |
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | |
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | |
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "MBD xsec : " << m_xsec_MBDNS << std::endl; | |
| std::cout << "total lumi (raw) : " << m_lumi_raw << std::endl; | |
| } | |
| m_bunchnumber_lumi_raw.fill(0); | |
| m_bunchnumber_lumi_live.fill(0); | |
| m_bunchnumber_lumi_scaled.fill(0); | |
| m_bunchnumber_crossings.fill(0); | |
| m_bunchnumber_MBDNS_raw.fill(0); | |
| m_bunchnumber_MBDNS_live.fill(0); | |
| m_bunchnumber_MBDNS_scaled.fill(0); | |
| m_lumi_raw = 0.; | |
| m_lumi_live = 0.; | |
| m_lumi_scaled = 0.; | |
| m_rawgl1scaler = 0; | |
| return Fun4AllReturnCodes::EVENT_OK; |
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | ||
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | ||
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); |
There was a problem hiding this comment.
Check m_streaming_lumi_info before writing output.
Line 263 through Line 265 dereference m_streaming_lumi_info unconditionally. If InitRun failed or node creation was skipped, this is a null-pointer crash path.
Proposed fix
- m_streaming_lumi_info->set_lumi_raw(get_lumi_raw());
- m_streaming_lumi_info->set_lumi_live(get_lumi_live());
- m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled());
+ if (!m_streaming_lumi_info)
+ {
+ std::cout << PHWHERE << " STREAMINGLUMIINFO node missing in EndRun" << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
+ m_streaming_lumi_info->set_lumi_raw(get_lumi_raw());
+ m_streaming_lumi_info->set_lumi_live(get_lumi_live());
+ m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled());As per coding guidelines: **/*.{cc,cpp,cxx,c} must prioritize correctness and memory safety.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | |
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | |
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); | |
| if (!m_streaming_lumi_info) | |
| { | |
| std::cout << PHWHERE << " STREAMINGLUMIINFO node missing in EndRun" << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| m_streaming_lumi_info->set_lumi_raw(get_lumi_raw()); | |
| m_streaming_lumi_info->set_lumi_live(get_lumi_live()); | |
| m_streaming_lumi_info->set_lumi_scaled(get_lumi_scaled()); |
| void StreamingLumiInfov1::Reset() | ||
| { | ||
| // Double check that this is only called once per run!! Or... it should just be called in the InitRun hook? | ||
| set_lumi_raw(0.); | ||
| set_lumi_live(0.); | ||
| set_lumi_scaled(0.); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Clarify Reset() semantics and intended usage.
The comment on line 9 indicates uncertainty about when Reset() should be called and whether it could be invoked multiple times. If this method clears per-run accumulated luminosity, unintended multiple calls could cause data loss. Please document the intended lifecycle of this object (per-event vs. per-run) and clarify whether Reset() should be called in InitRun(), between events, or elsewhere.
There was a problem hiding this comment.
Reset is called per event, so I don't think this is what you want (or at least, this won't do anything since you only fill these values within StreamingBcoLumiReco::EndRun which is correct)
There was a problem hiding this comment.
@osbornjd, thanks for the clarification — that makes sense. Since the lumi values are only populated in StreamingBcoLumiReco::EndRun(), the per-event Reset() calls throughout the run are hitting already-zeroed fields and have no effect on the accumulated data. My concern about data loss doesn't apply here. The lifecycle is correct as-is.
The in-code comment on line 9 could be replaced with a brief explanation of why it's a no-op during the run (e.g., "values are only set in EndRun; per-event Reset() is harmless"), but that's a minor style point — no functional issue.
✏️ Learnings added
Learnt from: osbornjd
Repo: sPHENIX-Collaboration/coresoftware PR: 4269
File: offline/packages/bcolumicount/StreamingLumiInfov1.cc:7-15
Timestamp: 2026-05-20T19:19:16.091Z
Learning: In `offline/packages/bcolumicount/StreamingLumiInfov1.cc`, `Reset()` is called per event by the Fun4All/PHObject framework. The lumi values (`lumi_raw`, `lumi_live`, `lumi_scaled`) are only populated once per run inside `StreamingBcoLumiReco::EndRun()`, so per-event `Reset()` calls are harmless no-ops during the run. Do not flag per-event Reset() calls on this object as a data-loss risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4268
File: offline/packages/CaloReco/ClusterCDFCalculator.cc:9-9
Timestamp: 2026-05-18T16:21:14.018Z
Learning: In the sPHENIX core software repository, C++20 std::format is supported (via the <format> header). During code review, do not flag uses of std::format / <format> as a potential C++-version or compiler-compatibility concern anywhere in the C++ codebase. (Other issues may still be flagged as appropriate.)
Build & test reportReport for commit ca045d501f0648671f39e68b316838fa7c04347c:
Automatically generated by sPHENIX Jenkins continuous integration |
| void StreamingBcoInfov1::Reset() | ||
| { | ||
| set_bco(0); | ||
| set_usable_bco_tag(false); | ||
| set_bco_streaming_window(std::make_pair(0, 0)); | ||
| return; |
There was a problem hiding this comment.
This is a relevant comment from the rabbit, we should reset all member variables of the EDM object. I think. Or perhaps the idea is to only set the event number once and have it be incremented on each process_event call? We may consider seeing if there is a "global" event number owned by fun4all that could be set here each reset call, so that it is guaranteed that this object is always in sync with Fun4all
| // TODO: Set BCO Negative window length in the macro | ||
| // window length: 360, negative window length: 20 | ||
| // Therefore, should check if BCO is within 340 | ||
| // special case if BCO is within 20 of previous BCO? |
There was a problem hiding this comment.
This should never happen, I think Cameron and I found there is a minimum allowed bco diff of 17 for triggered events which Chris can confirm - this is a real condition in the DAQ
| void StreamingLumiInfov1::Reset() | ||
| { | ||
| // Double check that this is only called once per run!! Or... it should just be called in the InitRun hook? | ||
| set_lumi_raw(0.); | ||
| set_lumi_live(0.); | ||
| set_lumi_scaled(0.); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Reset is called per event, so I don't think this is what you want (or at least, this won't do anything since you only fill these values within StreamingBcoLumiReco::EndRun which is correct)
| uint64_t bco_diff_prev = m_bco - bco_prev; | ||
| uint64_t bco_diff_futu = bco_futu - m_bco; | ||
|
|
||
| // TODO: Set BCO window length in the macro |
There was a problem hiding this comment.
Why don't we just do this now? We can set the default value to 340 and then have the necessary setter so that it can be modified if necessary
| bool m_usable_bco_tag = false; | ||
| std::pair<uint64_t, uint64_t> m_bco_streaming_window; | ||
|
|
||
| double m_xsec_MBDNS = 24.07*1e9; //convert to pb from Vernier scan DOUBLE CHECK VALUE! |
There was a problem hiding this comment.
We may consider having this be a global value in https://github.com/sPHENIX-Collaboration/coresoftware/blob/master/offline/framework/phool/sphenix_constants.h so that all code can use the same value and we only have to update one place
|
|
||
| int bunchno = packet->lValue(0,"BunchNumber"); | ||
|
|
||
| // SYNTAX TAKEN FROM ZHIWANS CODE, why = and not +=? If this is correct it seems like a waste to call it for every event (would just need it for the last event in a particular crossing?) |
2 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
(1) Add Subsys reco module (
StreamingBcoLumiReco) for producing DST fromBcoLumiRecooutput that contains the gl1 bco, the corresponding streaming window, a boolean to encode if the gl1 bco is usable for analysis, and the luminosity in the streaming data. (2) Add corresponding PHObject classes for storing the luminosity and gl1 info in the DST. (3) Add Subsys reco module to test new DST output.TODOs (if applicable)
PENDING DISCUSSION WITH THE BCO LUMI STREAMING GROUP -- we should discuss the current structure of the code and if this is what we want moving forward. For example, there are 2 passes over the gl1 raw data which might not be necessary. Other options to consider are:
BcoLumiReco, then we could just load the info directly from the DST. The relevant part ofStreamingBcoLumiRecois here.BcoLumiRecoprobably renaming toStreamingBcoLumiRecosuch that it outputs the equivalent of what is now output fromStreamingBcoLumiReco). I avoided this as the use ofSyncRecoand the way the bco info was being accessed inBcoLumiRecowas not as straightforward to me to work with for calculating the necessary quantities for the streaming bco lumi DST. However, now that the logic is in place inStreamingBcoLumiReco, it could be moved upstream if desired.Links to other PRs in macros and calibration repositories (if applicable)
Streaming BCO Luminosity Counting and DST Output
Motivation and Context
This PR adds streaming luminosity reconstruction based on BCO (Bunch Crossing Order) tagging and GL1 trigger scaler data. The new module produces a DST containing BCO-based luminosity information for analysis. This functionality was presented to the HF TG group and corresponds to macros PR sPHENIX-Collaboration/macros#1327.
Key Changes
New Reconstruction Module:
StreamingBcoLumiReco: Main SubsysReco module that reads GL1 packet 14001 (trigger scalers), extracts per-bunch and per-event BCO information, determines BCO usability for analysis, calculates streaming windows, and computes raw/live/scaled luminosities. Results are written to DST nodesSTREAMINGBCOINFOandSTREAMINGLUMIINFO.New PHObject Data Classes:
StreamingBcoInfo/StreamingBcoInfov1: Store GL1 BCO, event number, usable BCO flag, and streaming window (pair of uint64_t)StreamingLumiInfo/StreamingLumiInfov1: Store raw/live/scaled luminosity valuesTest/Validation Module:
StreamingBcoLumiCheck: Debug module to validate streaming DST output, logs luminosity values and BCO-related fieldsBuild System Updates:
Makefile.amto include new source files, ROOT dictionary generation, and updated library dependencies (-lfun4all)Removed:
BcoLumiCheckclass (replaced byStreamingBcoLumiCheck)Potential Risk Areas
STREAMINGBCOINFO,STREAMINGLUMIINFO) that downstream code must handlem_xsec_MBDNS). Bunch number wraparound logic (0–119 range) and abort gap handling (skips bunch indices >110) should be validatedBcoLumiReco, once inStreamingBcoLumiReco). Future optimization is plannedNotes for Reviewers
AI Disclaimer: AI-generated summaries may contain errors; use best judgment when reviewing code logic, especially the bunch number wraparound and luminosity normalization calculations.
The author notes pending discussion with the BCO lumi streaming group regarding code structure. Two refactoring approaches are proposed: (1) save lumi-calculation intermediates in BcoLumiReco DST for reuse, or (2) move streaming DST creation upstream into BcoLumiReco to avoid double-pass processing. Either approach could be adopted in future iterations.