Separate luminosity calculation#4317
Conversation
📝 WalkthroughWalkthroughThe PR splits ChangesBCO/Lumi Subsystem Split and Per-Bunch Lumi Data
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 |
dillfitz
changed the title
…tput of BcoLumiReco. Update lumi calculation code accordingly.
Build & test reportReport for commit 3e03d3b0b224f042d1943ad5a9ffab68ccb485e0:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 1ecfb4b1bbcfc1dafa05a83eea82500a57dc2a45:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
A few comments
- Is
StreamingBcoCheckjust intended to be a diagnostic module? Or am I missing its purpose? Same withStreamingLumiCheck? - Related to the above, I guess it is replacing
StreamingBcoLumiCheck? Why only delete the header file and not also the.ccfile?
I left some individual comments also. Some are rather large (e.g. moving from PRDF -> GL1RAWHIT) which we could delegate to another PR instead of adding it to this one
|
|
||
| class TH1; | ||
|
|
||
| class StreamingBcoReco : public SubsysReco |
There was a problem hiding this comment.
I think this is replacing StreamingBcoLumiReco.h but you didn't do a git mv StreamingBcoLumiReco.h StreamingBcoReco.h - is my understanding right? Then we should git rm the old file
There was a problem hiding this comment.
I think I did not use the cleanest workflow here. I started with the StreamingBcoLumiReco* files, made copies of them (just with cp), renamed the old and new set of files (just with mv), and then dealt with all of the git stuff when all was said and done just using git rm and git add. I must have missed one, and I don't know how git decided which files I renamed since I never used git mv. I can be more careful about this in the future.
I believe you are right and we can git rm StreamingBcoLumiReco.h
There was a problem hiding this comment.
After double checking, it looks like all the correct files are already there with no extras, though it looks very confusing how git decided to rename vs remove / add files. Ignoring the small changes to the StreamingLumiInfo class, I had to create 4 new files, and modify 4 existing files, basically by copying the 4 old StreamingBcoLumiReco and StreamingBcoLumiCheck files and renaming them accordingly. In the end, git decided that I added 5 files, removed 1, and renamed 3, which is effectively the same as renaming 4 and adding 4. I think git decided some files were just renamed based on how closely they resembled the old files -- but I only used git add and git rm.
| int StreamingLumiReco::process_event(PHCompositeNode *topNode) | ||
| { | ||
| StreamingBcoInfo *streaming_bcoinfo = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO"); | ||
| Event *evt = findNode::getClass<Event>(topNode, "PRDF"); |
There was a problem hiding this comment.
I just realized we have a problem here. If we read the raw PRDFs then we will never be able to delegate this to analyzers, because there is not enough disk space to keep the PRDFs disk resident. So we need to modify this to read from the GL1Packet object - I think the information content should be exactly analogous it is just reading from the "offline" GL1 object instead of the PRDF version. For reference, here is where the GL1 object gets filled in the event combiner - I think that should be enough to show you how to read the relevant information from this
That way the input for this module can be
- STREAMINGBCOINFO object in the "specially" made DST
- The GL1RAWHIT object, which we explicitly pass to every single DST so that the trigger/GL1 information is available at every level
There was a problem hiding this comment.
Yes this makes sense to me. We can also consider saving the trigger counters in the StreamingBcoInfo DST since it has to do a pass over the GL1 raw data, but I am not sure if that workflow makes as much sense.
There was a problem hiding this comment.
Thanks for the review. I have addressed your other comments. Do you prefer we address this issue in this PR or a separate one?
For reference, the corresponding macros PR has already been merged (sPHENIX-Collaboration/macros#1347)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/bcolumicount/StreamingLumiCheck.cc (1)
42-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
InitRunis too early to validate final lumi output.The PR moves luminosity publication into
StreamingLumiReco's end-of-run path, so readingSTREAMINGLUMIINFOhere can only see default or stale values. Because the null case also returnsEVENT_OK, this checker can silently pass while never validating the actual reconstructed lumi. Please move this dump to a post-computation hook with node access (for exampleEnd(PHCompositeNode*)) and fail loudly ifSTREAMINGLUMIINFOis missing there.As per path instructions, this validator should prioritize correctness and error handling.
Source: Path instructions
offline/packages/bcolumicount/StreamingBcoReco.cc (1)
167-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the window start against unsigned underflow.
get_bco() - m_default_negative_window_lengthis evaluated asuint64_t. For low or wrapped BCO values, the published window start jumps to a huge number, andStreamingLumiRecowill reconstruct the wrong crossing range for that event. Please clamp or handle wraparound before storingm_bco_streaming_window. As per path instructions,**/*.{cc,cpp,cxx,c}reviews should prioritize correctness, memory safety, error handling, and thread-safety.Source: Path instructions
♻️ Duplicate comments (1)
offline/packages/bcolumicount/StreamingLumiReco.cc (1)
82-103: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThis module still cannot run from DST-only inputs.
process_eventstill aborts unless thePRDFnode and packet 14001 are present, so the new lumi stage remains tied to raw input availability instead of the persisted GL1 object. That was the earlier blocker for delegating this step to analyzer-style DST workflows. This was already raised in the earlier review thread. As per path instructions,**/*.{cc,cpp,cxx,c}reviews should prioritize correctness, memory safety, error handling, and thread-safety.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c708720-1aeb-44ee-b9c8-3f2ab588567f
📒 Files selected for processing (12)
offline/packages/bcolumicount/Makefile.amoffline/packages/bcolumicount/StreamingBcoCheck.ccoffline/packages/bcolumicount/StreamingBcoCheck.hoffline/packages/bcolumicount/StreamingBcoLumiCheck.hoffline/packages/bcolumicount/StreamingBcoReco.ccoffline/packages/bcolumicount/StreamingBcoReco.hoffline/packages/bcolumicount/StreamingLumiCheck.ccoffline/packages/bcolumicount/StreamingLumiCheck.hoffline/packages/bcolumicount/StreamingLumiInfo.hoffline/packages/bcolumicount/StreamingLumiInfov1.hoffline/packages/bcolumicount/StreamingLumiReco.ccoffline/packages/bcolumicount/StreamingLumiReco.h
💤 Files with no reviewable changes (1)
- offline/packages/bcolumicount/StreamingBcoLumiCheck.h
| int StreamingBcoCheck::process_event(PHCompositeNode *topNode) | ||
| { | ||
| StreamingBcoInfo *streaming_bco_info = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO"); | ||
| if (streaming_bco_info) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << "bco : " << streaming_bco_info->get_bco() << std::endl; | ||
| std::cout << "usable bco tag : " << streaming_bco_info->get_usable_bco_tag() << std::endl; | ||
| std::cout << "bco streaming window : (" << streaming_bco_info->get_bco_streaming_window().first << ", " << streaming_bco_info->get_bco_streaming_window().second << ")" << std::endl; | ||
| } | ||
| } | ||
|
|
||
| return Fun4AllReturnCodes::EVENT_OK; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast when STREAMINGBCOINFO is missing.
This checker currently returns EVENT_OK even when its required input node is absent, so a broken StreamingBcoReco/checker wiring silently looks healthy. StreamingBcoReco's downstream contract treats a missing STREAMINGBCOINFO as an abort condition, so the validator should do the same here.
Suggested fix
int StreamingBcoCheck::process_event(PHCompositeNode *topNode)
{
StreamingBcoInfo *streaming_bco_info = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO");
- if (streaming_bco_info)
- {
- if (Verbosity() > 1)
- {
- std::cout << "bco : " << streaming_bco_info->get_bco() << std::endl;
- std::cout << "usable bco tag : " << streaming_bco_info->get_usable_bco_tag() << std::endl;
- std::cout << "bco streaming window : (" << streaming_bco_info->get_bco_streaming_window().first << ", " << streaming_bco_info->get_bco_streaming_window().second << ")" << std::endl;
- }
- }
+ if (!streaming_bco_info)
+ {
+ std::cout << PHWHERE << " STREAMINGBCOINFO node missing" << std::endl;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
+
+ if (Verbosity() > 1)
+ {
+ std::cout << "bco : " << streaming_bco_info->get_bco() << std::endl;
+ std::cout << "usable bco tag : " << streaming_bco_info->get_usable_bco_tag() << std::endl;
+ std::cout << "bco streaming window : (" << streaming_bco_info->get_bco_streaming_window().first
+ << ", " << streaming_bco_info->get_bco_streaming_window().second << ")" << std::endl;
+ }
return Fun4AllReturnCodes::EVENT_OK;
}As per path instructions, this code path should 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.
| int StreamingBcoCheck::process_event(PHCompositeNode *topNode) | |
| { | |
| StreamingBcoInfo *streaming_bco_info = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO"); | |
| if (streaming_bco_info) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "bco : " << streaming_bco_info->get_bco() << std::endl; | |
| std::cout << "usable bco tag : " << streaming_bco_info->get_usable_bco_tag() << std::endl; | |
| std::cout << "bco streaming window : (" << streaming_bco_info->get_bco_streaming_window().first << ", " << streaming_bco_info->get_bco_streaming_window().second << ")" << std::endl; | |
| } | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| int StreamingBcoCheck::process_event(PHCompositeNode *topNode) | |
| { | |
| StreamingBcoInfo *streaming_bco_info = findNode::getClass<StreamingBcoInfo>(topNode, "STREAMINGBCOINFO"); | |
| if (!streaming_bco_info) | |
| { | |
| std::cout << PHWHERE << " STREAMINGBCOINFO node missing" << std::endl; | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "bco : " << streaming_bco_info->get_bco() << std::endl; | |
| std::cout << "usable bco tag : " << streaming_bco_info->get_usable_bco_tag() << std::endl; | |
| std::cout << "bco streaming window : (" << streaming_bco_info->get_bco_streaming_window().first << ", " << streaming_bco_info->get_bco_streaming_window().second << ")" << std::endl; | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; |
Source: Path instructions
| virtual void set_default_positive_window_length(int val) { m_default_positive_window_length = val; } | ||
| virtual void set_default_negative_window_length(int val) { m_default_negative_window_length = val; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative window lengths at the API boundary.
These setters take int, but the stored members are unsigned int (Lines 48-49). A negative value from a macro/config silently wraps to a multi-billion-crossing window and poisons the downstream BCO/lumi windowing. If the signed signature is needed for compatibility, please range-check before assignment; the same issue is mirrored in StreamingLumiReco.h. As per path instructions, **/*.{h,hpp,hxx,hh} reviews should focus on API clarity/stability and only raise Critical or Major findings.
Source: Path instructions
| int lower = streaming_bcoinfo->get_bco_streaming_window().first - streaming_bcoinfo->get_bco(); | ||
| int upper = streaming_bcoinfo->get_bco_streaming_window().second - streaming_bcoinfo->get_bco(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compute the window offsets in a signed type.
get_bco_streaming_window().first/second and get_bco() are uint64_t. Subtracting them here performs unsigned arithmetic first and only then narrows to int, so the negative lower bound currently depends on implementation-defined wraparound. A compiler/ABI change here would miscount bunch crossings. As per path instructions, **/*.{cc,cpp,cxx,c} reviews should prioritize correctness, memory safety, error handling, and thread-safety.
Source: Path instructions
| 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 * sphenix_constants::m_xsec_MBDNS ); | ||
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); | ||
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep rawgl1scalers_per_bunch as double and guard zero.
m_rawgl1scaler / 120. is immediately truncated back into uint64_t. Any run with fewer than 120 raw counts makes the denominator zero and the three lumi calculations divide by zero; larger runs are still biased low by the truncation. As per path instructions, **/*.{cc,cpp,cxx,c} reviews should prioritize correctness, memory safety, error handling, and thread-safety.
Suggested fix
- uint64_t rawgl1scalers_per_bunch = m_rawgl1scaler/120.;
+ const double rawgl1scalers_per_bunch =
+ static_cast<double>(m_rawgl1scaler) / static_cast<double>(m_bunches);
+ if (rawgl1scalers_per_bunch <= 0.)
+ {
+ std::cout << PHWHERE << " invalid raw GL1 scaler total: " << m_rawgl1scaler << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }📝 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 * sphenix_constants::m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); | |
| const double rawgl1scalers_per_bunch = | |
| static_cast<double>(m_rawgl1scaler) / static_cast<double>(m_bunches); | |
| if (rawgl1scalers_per_bunch <= 0.) | |
| { | |
| std::cout << PHWHERE << " invalid raw GL1 scaler total: " << m_rawgl1scaler << 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 * sphenix_constants::m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_live[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_live[i]) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); | |
| m_bunchnumber_lumi_scaled[i] = ( m_bunchnumber_crossings[i] * m_bunchnumber_MBDNS_scaled[i] ) / ( rawgl1scalers_per_bunch * sphenix_constants::m_xsec_MBDNS ); |
Source: Path instructions
Build & test reportReport for commit 0fa4abb137678f309e43673773ce0943f5284b66:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
Separate luminosity calculation into its own Subsys reco module (StreamingLumiReco)
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
It is a refactoring of an existing feature -- separating the streaming luminosity calculation from the trigger bco validation code.
This follows work started in http://github.com/sPHENIX-Collaboration/coresoftware/pull/4269.
TODOs (if applicable)
StreamingBcoRecowill be passed unmodified to the lumi calculation (as they are now). My understanding is that they need to go through a QA step first, so we may need to modify the sequence in the future.process_eventloop inStreamingLumiCheckis okay. In principal it is not needed, the relevant checks take place right in theInitRunsequence. If so, remove commented out block (it was left in for visibility)Here is the current sequence:
graph TD; BcoLumiReco-->StreamingBcoReco; StreamingBcoReco-->StreamingLumiReco; StreamingBcoReco-->Analyst; StreamingLumiReco-->Analyst;I believe in the future it will be
graph TD; BcoLumiReco-->StreamingBcoReco; StreamingBcoReco-->StreamingDetectorQA; StreamingDetectorQA-->StreamingLumiReco; StreamingBcoReco-->Analyst; StreamingLumiReco-->Analyst;Links to other PRs in macros and calibration repositories (if applicable)
sPHENIX-Collaboration/macros#1347
Motivation / context
StreamingLumiRecosubsystem, separating luminosity calculation from BCO validation logic.Key changes
StreamingLumiRecoas a new reconstruction module for luminosity calculation.StreamingLumiRecoto consumeStreamingBcoRecooutput instead ofBcoLumiReco.StreamingBcoReco/StreamingBcoCheckand removedInitRunfromStreamingBcoReco.StreamingLumiCheck/StreamingBcoCheckheaders and implementations to reflect the new module boundaries.StreamingLumiInfo/StreamingLumiInfov1with per-bunch lumi storage/accessors.Makefile.amfor the new/renamed headers and sources.Potential risk areas
Possible future improvements
process_eventloop inStreamingLumiCheckif it is no longer needed.AI-generated summaries can make mistakes; please use best judgment when reviewing the changes.