Skip to content

Adding Centrality to KFParticle - #4456

Closed
BrentMartin-ISU wants to merge 0 commit into
sPHENIX-Collaboration:masterfrom
BrentMartin-ISU:master
Closed

BrentMartin-ISU wants to merge 0 commit into
sPHENIX-Collaboration:masterfrom
BrentMartin-ISU:master

Conversation

@BrentMartin-ISU

@BrentMartin-ISU BrentMartin-ISU commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

Adding the ability for KFParticle to display and calculate centrailty data

TODOs (if applicable)

Finish implementation and test to confirm accuracy then share with collaboration

Links to other PRs in macros and calibration repositories (if applicable)

Motivation

Add MBD centrality to KFParticle decay-tree output so analyses can use centrality with reconstructed data. The draft description says implementation and accuracy testing remain incomplete.

Key changes

  • Add the opt-in useCentrality() setter and m_use_centrality_nTuple flag.
  • When centrality output is enabled, add the centrality_mbd tree branch.
  • Read the mbd_NS bin from CentralityInfo during event filling.
  • Set centrality to -1 if CentralityInfo is missing or any daughter has a nonzero bunch crossing. Set it to -2 if the node lacks the mbd_NS bin.
  • Link the KFParticle sPHENIX I/O library against -lcentrality.

Potential risk areas

  • I/O format: The optional centrality_mbd branch changes the tree schema when enabled. Downstream readers may need to handle it.
  • Reconstruction behavior and data interpretation: The code accepts centrality only when all daughters have bunch crossing zero. Consumers must also treat -1 and -2 as sentinel values, not physical centralities.
  • No supplied evidence establishes changes to thread safety or performance.

Possible future improvements

  • Complete implementation and validate the output against an independent centrality reference.
  • Document the CentralityInfo node, mbd_NS bin, bunch-crossing condition, and sentinel values.
  • Confirm the configuration path that enables centrality output.

AI-generated change descriptions can contain errors. Use best judgment and verify these details against the source and collaboration review.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

KFParticle_sPHENIX adds an option to enable centrality output. When enabled, KFParticle_nTuple adds a centrality_mbd tree branch and fills it from CentralityInfo. The value depends on node availability, the mbd_NS bin, and daughter bunch crossings. Ntuple calculations now use the class’s m_tools_nTuple member.

Changes: KFParticle ntuple updates

Layer / File(s) Summary
Add the centrality option and state
offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.h, offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h, offline/packages/KFParticle_sPHENIX/Makefile.am, offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc
useCentrality sets the centrality option stored by KFParticle_nTuple. The centrality value starts at -1, and the I/O library links against -lcentrality. The tuple adds a centrality_mbd branch when centrality output is enabled.
Use the KFParticle tools member
offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h, offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc, offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc
Ntuple calculations use m_tools_nTuple instead of a file-scope tools object. Both constructors assign the current object to m_tools_nTuple.
Fill centrality from the event
offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc
When centrality is enabled, event filling reads the mbd_NS bin from CentralityInfo. It sets the value to -1 if the node is missing or any daughter has a nonzero bunch crossing, and to -2 if the bin is unavailable.

Sequence Diagram(s)

sequenceDiagram
  participant KFParticle_nTuple
  participant CentralityInfo
  participant TTree
  KFParticle_nTuple->>TTree: Adds centrality_mbd branch when enabled
  KFParticle_nTuple->>CentralityInfo: Reads mbd_NS bin
  KFParticle_nTuple->>TTree: Stores centrality_mbd value
Loading

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 5e348

Standalone ntuple use can crash when filling a branch. Initialize its tools dependency before merging, or explicitly accept that limitation.

Security architecture risk: 🔵 Low · up to 5e348

The feature is opt-in and uses existing event data within the reconstruction package. No introduced security vulnerability was established. Downstream output compatibility and unusual object-lifecycle behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated new data path is confined to the running reconstruction object and its configured ROOT output: centrality is read from event state and copied into a tuple field. The inspected path does not write centrality back into shared event state; external consumer and deployment exposure remain unestablished.

Trust Boundaries and Controls

  • observed — The centrality path checks node availability, bin availability, and daughter bunch crossings before accepting the value. These are output-validity controls, not authentication or authorization controls; the value remains supplied by an existing event-node producer.

Resilience and Maintainability Implications

  • observed — Under a stable enabled configuration, every shown centrality outcome assigns the scalar before row publication, including missing-node, missing-bin, and invalid-crossing outcomes. This prevents those normal failure paths from reusing the previous candidate's centrality value.

Hardening Proposals

  • proposed — Make the supported lifecycle constraints explicit: keep centrality selection fixed after branch initialization, and either prohibit owner copying or define complete rebinding and output-handle semantics if copying is intended. This is preventive hardening, not an established attacker-reachable defect.
- [ ] Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit f64dcf5a9e06750edf69166d592ab7ee8670e7c4:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 1cb535cdc300685c48fce62ab91ec036457bf006:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 376c1f0319779e1070da2974507cffbd89b827d7:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 2793cd35d0e064eeae4c30316504e30ad1fac21e:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 59aa07e823371a7141ef6c8dbc0eecd96b9801e5:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: sPHENIX-Collaboration/coresoftware/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 52618145-ba11-47dd-9ac4-21356ba7108d
📥 Commits

Reviewing files that changed from the base of the PR and between 59aa07e and 6bca07f.

📒 Files selected for processing (3)
  • offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc
  • offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h
  • offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: sPHENIX-Collaboration/coresoftware/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1d7cd4d6-fddb-476d-ae1d-4fca2ac903c0
📥 Commits

Reviewing files that changed from the base of the PR and between 6bca07f and 5e34808.

📒 Files selected for processing (2)
  • offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc
  • offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc Outdated
@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit de68d38eb591e4526022c10a5ad09ab5479bd2de:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 6bca07f332f8585939b3d7bd370f2f8f4dc34247:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit de6e60e411495aab059195f0426001ca40366f76:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 2a5dac89d94089dcbfaa9c005fa73d18a9129753:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 5e3480884b805fe30ef99cd9390ca790633789ef:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 700ee458cb49f81879d90351129628a20202b48c:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 7c72c7821bf80de340fbb2b473fea1b8e963325f:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 9e233d9981f613e0532f5bf1a87aa5cff4075054:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 0ddfeaf172b07f88b329b898b5ea4f486f3883b3:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@osbornjd

osbornjd commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Your PR is failing the CI because of compilation errors, see
https://web.sdcc.bnl.gov/jenkins-sphenix/job/sPHENIX/job/Build-Master-gcc14/3111//artifact/build/new/rebuild.log
Were you able to build this locally?

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 778ceb618f699997d8742f80f9f0e0f9cf6686fb:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit e28baba7f1ca8f175e222105e70c322028354a7a:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 643cf9be14a154405851a8a3c0feb1f1ccb6a4d7:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 9e2182956b757a2756e4229e043fd02d212b6819:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 287c6c31d101e52f856dccf7cf67ab0778be81db:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit d637ab4caeeae31fd33cf0f040857bab3dc94daa:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit c1b07af8fbd238b590b217870042698108130f56:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit a80548cbe20ec433ce4956f562a43bc288e58f75:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit ba5bd66a3d55d785556c28ab114d0098a681b9c6:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants