Skip to content

updated MB classifier for O+O and species dependence - #4355

Merged
pinkenburg merged 5 commits into
sPHENIX-Collaboration:masterfrom
pjwinnetou:master
Jul 24, 2026
Merged

updated MB classifier for O+O and species dependence#4355
pinkenburg merged 5 commits into
sPHENIX-Collaboration:masterfrom
pjwinnetou:master

Conversation

@pjwinnetou

@pjwinnetou pjwinnetou commented Jul 21, 2026 •
edited by coderabbitai Bot
Loading

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, ...)

TODOs (if applicable)

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

Motivation / context

Update the minimum-bias (MB) classifier to better support O+O collisions with species-dependent selection criteria, including optional pileup rejection, and address event-centrality consistency issues when an event fails the MB classifier.

Key changes

  • MinimumBiasClassifier (offline/packages/trigger/MinimumBiasClassifier.cc / .h)

    • Species-dependent cut initialization in InitRun using an if/else-if split:
      • For O+O: sets m_max_charge_cut=400 (was 300), enables additional vertex/MBd-related thresholds (including m_z_vtx_cut), and initializes MBD charge/time cuts.
      • For p+p: keeps the simpler baseline setup while initializing the relevant MBD charge/time cuts.
    • OO-only pileup rejection in FillMinimumBiasInfo: when m_reject_pileup is enabled and total summed MBD charge exceeds m_pileup_charge_cut, minbiascheck is forced false.
    • Correct Fun4All return-code propagation: process_event() now returns GetNodes(topNode) non-EVENT_OK codes directly.
    • Reduced node-missing logging: removed an unnecessary log message when the GLOBAL node is absent.
    • New configuration knob: added setRejectPileup(bool) and the associated per-instance state (m_reject_pileup, m_pileup_charge_cut).
    • Cut values moved to per-instance members (instead of static constexpr constants) to support per-instance configuration.

  • Centrality consistency fixes

    • CentralityReco::process_event() now calls m_central->Reset() before the MB check, preventing centrality from carrying over from the previous event when MB fails.
    • CentralityInfov2::Reset() is now properly implemented to reset base state and clear _centrality_bin_map, ensuring v2 centrality bin info doesn’t persist across resets.

Potential risk areas

  • Physics/event selection impact: changed MB thresholds for O+O and introduced OO pileup veto behavior can alter accepted event fractions and downstream physics yields.
  • Centrality output behavior changes: centrality is now reset earlier—events failing MB will no longer retain previous-event centrality values.
  • Configuration/state safety: per-instance cut parameters and the new setRejectPileup setter should be reviewed for any shared-instance/threading assumptions.
  • Behavior on missing nodes: process_event() now propagates GetNodes() return codes directly, which may affect how failures are handled/aborted in the running framework.

Possible future improvements

  • Move remaining “magic-number” MB cut defaults (e.g., pileup/MBD/vertex thresholds) into CDB/calibration inputs as requested by reviewers, rather than hardcoded per-instance values.
  • Add regression/unit tests covering:
    • OO pileup rejection boundary conditions (m_pileup_charge_cut just above/below)
    • centrality reset behavior for MB-failing events
    • return-code handling for missing/invalid detector nodes.
  • Validate MB efficiency and rejection rates for O+O against reference data/sim.

Note: AI-generated summaries can contain errors—please use these details as a starting point and rely on direct code/physics validation when making decisions.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@pjwinnetou, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e76d710-0632-4e0a-8dc8-1d992db5ded8

📥 Commits

Reviewing files that changed from the base of the PR and between 87658d9 and de59c12.

📒 Files selected for processing (1)
  • offline/packages/centrality/CentralityReco.cc
📝 Walkthrough

Walkthrough

MinimumBiasClassifier adds per-instance cut configuration, expanded OO/PP thresholds, and configurable OO pileup rejection. Centrality processing now resets stored state and propagates node errors accurately.

Changes

Event processing updates

Layer / File(s) Summary
Classifier configuration and species thresholds
offline/packages/trigger/MinimumBiasClassifier.h, offline/packages/trigger/MinimumBiasClassifier.cc		 Adds pileup configuration, converts cut constants to instance members, initializes expanded OO and PP thresholds, and rejects OO events above the configured MBD charge threshold.
Centrality state reset
offline/packages/centrality/CentralityInfov2.h, offline/packages/centrality/CentralityInfov2.cc, offline/packages/centrality/CentralityReco.cc		 Implements CentralityInfov2::Reset(), clears centrality-bin mappings, and invokes the reset during event processing.
Node status propagation and logging
offline/packages/trigger/MinimumBiasClassifier.cc		 Propagates non-EVENT_OK node-retrieval status and removes the missing GLOBAL node log message.

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.

❤️ Share

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

@pinkenburg

pinkenburg commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

The GetNodes(topNode) method uses Fun4All return codes which then get reinterpreted to EVENT_OK in the process_event(). That is confusing and likely not what you want. In this case the classifier does not work and returning ok will just spew warnings for every single event (blowing up condor logs). If Fun4All return codes are used as return from GetNodes() they have to be passed up, otherwise change the return code to some classifier specific one and return the proper Fun4All return code from process_event()

many of the hardcoded parameters in this list
m_useZDC = true;
m_box_cut = true;
m_hit_cut = 2;
m_max_charge_cut = 2100;
m_mbd_charge_cut = 0.5;
m_mbd_time_cut = 25.;
m_z_vtx_cut = 60.;
m_mbd_north_cut = 10.;
m_mbd_south_cut = 150.;
m_zdc_cut = 60.;
are valid for all species but they are set individually for every species. That invites cut and paste errors - initialize them to a common default and only change the ones which are different. I would feel more comfortable if this would be offloaded to the cdb instead of using magic numbers
The printout in line 379 is not relevant, this is default behavior (if the detector node does not exist, create it) and shouldn't fill our logs. If this is unexpected - it should be reflected in the printout

@bseidlit

bseidlit commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Can you fix the bug in the centrality reco while you are at it? The fact that the centrality reports the previous events centrality if the current event does not pass the minbias classifier.

@sphenix-jenkins-ci

sphenix-jenkins-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown

Build & test report

Report for commit 0fe9dc0ece4c7a464f6b7e03dfb1b3d412236ed1:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

sphenix-jenkins-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown

Build & test report

Report for commit 8bc5f37c959027030b879cfbc51c39302dcf60d4:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pjwinnetou

pjwinnetou commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@pinkenburg I updated the structure. Could you take a look and see if this is okay to go?
@bseidlit it seems it was a problem in the reset. I updated the centrality reco module

pinkenburg
pinkenburg reviewed Jul 23, 2026
View reviewed changes
Comment thread offline/packages/centrality/CentralityReco.cc Outdated
return ret;
}

m_central->Reset();

@pinkenburg pinkenburg Jul 23, 2026

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.

This is not necessary. The Reset() is called for all objects on the node tree after an event is processed. The problem here was that the centrality info object implemented an empty reset method so the node tree reset did not reset it. This line here should be removed to avoid confusion

pjwinnetou and others added 2 commits July 23, 2026 16:09
@pjwinnetou
Merge branch 'sPHENIX-Collaboration:master' into master
87658d9
JAEBEOm PARK @claude
remove redundant explicit Reset in CentralityReco::process_event
de59c12 
The node tree reset already calls CentralityInfov2::Reset() after each
event now that it is properly implemented.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sphenix-jenkins-ci

sphenix-jenkins-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown

Build & test report

Report for commit 87658d988856868999b7721af221991910649801:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

sphenix-jenkins-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown

Build & test report

Report for commit de59c12ae8ed425092b2d14bdd3c85e2b2bc46ce:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg
pinkenburg merged commit ec88957 into sPHENIX-Collaboration:master Jul 24, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pinkenburg pinkenburg pinkenburg left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pjwinnetou @pinkenburg @bseidlit