MinBias Class Abort and Centrality Node naming#4247
Open
danjlis wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
Open
MinBias Class Abort and Centrality Node naming#4247danjlis wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
danjlis wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis change refactors node name handling in the centrality and trigger subsystems, replacing hardcoded node identifiers with configurable member variables accessible through public setter methods. Additionally, event abortion logic is introduced to MinimumBiasClassifier. Changes
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 cb8f934332e6d40f3ef7d440cee6b6bab175a705:
Automatically generated by sPHENIX Jenkins continuous integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1dc46d3e-6d55-4c9a-8b3d-57d53dfc3577
📒 Files selected for processing (4)
offline/packages/centrality/CentralityReco.ccoffline/packages/centrality/CentralityReco.hoffline/packages/trigger/MinimumBiasClassifier.ccoffline/packages/trigger/MinimumBiasClassifier.h
| #include <string> // for string, allocator | ||
|
|
||
| // Forward declarations | ||
| class TF1; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is m_use_vtx_function actually read anywhere?
rg -nP '\bm_use_vtx_function\b' -C2
# Is TF1 or CDBTF used anywhere in the centrality package?
rg -nP '\bTF1\b|\bCDBTF\b' -C2 -g 'offline/packages/centrality/**'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1242
🏁 Script executed:
# Search for any actual usage/assignment of m_use_vtx_function (not just declaration)
rg -n 'm_use_vtx_function' offline/packages/centrality/ -A2 -B2
# Search for actual instantiation or usage of TF1 objects
rg -n 'TF1\s*[\(\[]|new\s+TF1|\..*TF1' offline/packages/centrality/ -C2
# Search for CDBTF usage
rg -n 'CDBTF\s*[\(\[]|new\s+CDBTF|CDBTF::' offline/packages/centrality/ -C2
# Check the full CentralityReco.cc to see the complete implementation context
wc -l offline/packages/centrality/CentralityReco.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 467
🏁 Script executed:
# Read the full CentralityReco.cc to check for actual usage
cat -n offline/packages/centrality/CentralityReco.cc | head -50
# Also check if there are any setter/getter methods for m_use_vtx_function
rg -n 'use_vtx_function|UseVtxFunction|setUseVtxFunction' offline/packages/centrality/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1630
🏁 Script executed:
# Read more of the implementation to see if TF1 or m_use_vtx_function are used anywhere
sed -n '41,200p' offline/packages/centrality/CentralityReco.cc | cat -n
# Check process method and remaining code
sed -n '200,388p' offline/packages/centrality/CentralityReco.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 10825
Remove the unused TF1 forward-declaration, includes, and member toggle, or complete the implementation.
The class TF1; forward-decl (line 11), #include <TF1.h>, #include <cdbobjects/CDBTF.h>, and the m_use_vtx_function{true} member (line 96) are present but completely unused in the implementation. Shipping a public member variable that defaults to true but has no effect risks users mistaking it for working functionality and complicates future real implementation efforts.
Either complete the vertex function feature in this PR or defer it by removing these declarations until the implementation is ready.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit includes setting node names and allowing MBClassifier to abort events if directed to do so
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)
PR Summary: MinBias Class Abort and Centrality Node Naming
Motivation / Context
This PR adds configurable node naming for reconstruction modules and event abort functionality to the minimum-bias trigger classifier. These features enable users to use non-standard node names when operating multiple centrality and trigger reconstruction chains in parallel, and allow event filtering based on minimum-bias criteria.
Key Changes
CentralityReco (node naming):
set_minbiasNodeName(),set_mbdOutNodeName(),set_centralityNodeName(),set_mbdPmtNodeName()GetNodes()to retrieveMinimumBiasInfo,CentralityInfo,MbdPmtContainer, andMbdOutfrom configurable node name member variables instead of hardcoded string literalsCreateNodes()to create the outputCentralityInfonode using the configurable name variableGlobalVertexMapretrieval requirement (previously a no-op when node was missing)m_use_vtx_functionflag andTF1forward declarationMinimumBiasClassifier (abort events + node naming):
CreateNodes()from static to non-static member methodabortEvents(bool)to enable/disable event abortionset_minbiasNodeName(),set_mbdPmtNodeName(),set_zdcInfoNodeName(),set_globalvertexNodeName()FillMinimumBiasInfo()to return integer status codes (0/1) instead ofFun4AllReturnCodes::EVENT_OK, with abortion gated bym_abortEventsflagprocess_event()to abort events (ABORTEVENT) whenFillMinimumBiasInfo()returns nonzero andabortEvents()is enabledGetNodes()andCreateNodes()to use configurable node names instead of hardcoded valuesPotential Risk Areas
abortEvents(true)is explicitly set. Default isfalse(preserving current behavior), but new code enabling this may filter events unexpectedly if criteria are not properly configured.CreateNodes()changed from static to non-static inMinimumBiasClassifier. Any out-of-tree or existing code calling this as a static method will break at compile time."MinimumBiasInfo","CentralityInfo","MbdPmtContainer","MbdOut","Zdcinfo","GlobalVertexMap"). Non-backward-compatible if external code hardcodes node creation or retrieval.GlobalVertexMap. Verify this removal doesn't break downstream code that previously relied on its presence.Possible Future Improvements