Skip to content

feat(MinimumBiasClassifier): Add PHParameters for detailed cut tracking#3884

Draft
Steepspace wants to merge 6 commits intosPHENIX-Collaboration:masterfrom
Steepspace:MinBias
Draft

feat(MinimumBiasClassifier): Add PHParameters for detailed cut tracking#3884
Steepspace wants to merge 6 commits intosPHENIX-Collaboration:masterfrom
Steepspace:MinBias

Conversation

@Steepspace
Copy link
Contributor

@Steepspace Steepspace commented Sep 19, 2025

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

This PR introduces PHParameters to the MinimumBiasClassifier to provide detailed, event-by-event information about why the minimum bias selection passes or fails. Previously, the module only set a single boolean, making it difficult to debug or perform quality assurance checks.

Key changes include:

  • A PHParameters object named "MinBiasParams" is now created and stored on the PAR node.
  • Individual boolean flags are set to true if an event fails a specific selection criterion, such as:
    • minbias_background_cut_fail
    • minbias_two_hit_min_fail
    • minbias_zdc_energy_min_fail
    • minbias_mbd_total_energy_max_fail
  • Key calculated values (e.g., MBD north/south charge, vertex scale) are also saved to this object for downstream analysis and QA.
  • The total MBD charge cut is no longer a hardcoded constant and can be configured via the new set_mbd_total_charge_cut() method. (Note: The default total MBD charge cut of 2100 remains unchanged.)

TODOs (if applicable)

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

@sphenix-jenkins-ci
Copy link

sphenix-jenkins-ci bot commented Sep 20, 2025

Build & test report

Report for commit 2af2be95566efb8e8f5eb2dda64ab85a79543791:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@Steepspace Steepspace marked this pull request as ready for review September 20, 2025 05:55
@pinkenburg
Copy link
Contributor

pinkenburg commented Sep 20, 2025 •
edited
Loading

PHParameters are very expensive to use on an event by event basis. They are meant to be used for configurations and have a DB backend. This is just the wrong class to handle event by event parameters in this case,

@Steepspace
Copy link
Contributor Author

Steepspace commented Sep 20, 2025

PHParameters are very expensive to use on an event by event basis. They are meant to be used for configurations and have a DB backend. This is just the wrong class to handle event by event parameters in this case,

I will note that PHParameters is currently used in the JetBackgroundCut module that also does something similar on an event by event basis:

coresoftware/offline/packages/jetbackground/JetBackgroundCut.cc

Lines 283 to 293 in 9d03281

PHCompositeNode *parNode = dynamic_cast<PHCompositeNode *>(iter.findFirst("PHCompositeNode", "PAR"));
_cutParams.set_int_param("failsLoEmJetCut", failsLoEm);
_cutParams.set_int_param("failsHiEmJetCut", failsHiEm);
_cutParams.set_int_param("failsIhJetCut", failsIhCut);
_cutParams.set_int_param("failsAnyJetCut", failsAnyCut);
_cutParams.set_int_param("isDijet", isDijet);
_cutParams.set_double_param("frcem", frcem);
_cutParams.set_double_param("frcoh", frcoh);
_cutParams.set_double_param("maxJetET", maxJetET);
_cutParams.set_double_param("dPhi", dPhi);
_cutParams.UpdateNodeTree(parNode, "JetCutParams");

However, since this is not recommended, do you suggest any other approach that will be ideal to pass the information regarding the cuts so that QA downstream can log that different cases?

@pinkenburg
Copy link
Contributor

pinkenburg commented Sep 20, 2025

That was a bit of a temporary measure - before that they used PHFlag which is even worse. The very basic problem here (and in the jet background) is that it uses the temporary "PAR" which is not saved (and also not reset - so there is a possibility of carrying information from one event to the next accidentally). That means the downstream qa module can never be run off a DST which is just a bad idea (and it will potentially read info from a previous event). I don't see any qa code which accesses this node - so I am not sure if this was actually thought out or followed up with an actual qa implementation.
What we probably need is some dumb i/o class with an int and double vector and then have accessors for each implementation which just look values up via index and store this thing under the DST node
.

@Steepspace
Copy link
Contributor Author

Steepspace commented Sep 20, 2025

What we probably need is some dumb i/o class with an int and double vector and then have accessors for each implementation which just look values up via index and store this thing under the DST node

Thank you for the suggestion. Could you provide a template or reference that I can use to implement this i/o class that can store these quantities under the DST node?
In the meantime, please feel free to mark this PR back to Draft since it's not acceptable at this stage.

@pinkenburg pinkenburg marked this pull request as draft September 20, 2025 23:23
@sphenix-jenkins-ci
Copy link

sphenix-jenkins-ci bot commented Oct 31, 2025

Build & test report

Report for commit ef1c109a81e6aeebdf52f3a02261fea1c7da033b:
Jenkins on fire

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci
Copy link

sphenix-jenkins-ci bot commented Nov 14, 2025

Build & test report

Report for commit 6aa5d2a73f9ba02afcc703a4bca98ae19d5351e5:
Jenkins on fire

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@Steepspace
Update MinimumBiasClassifier - Add PHParamters
752589e 
- No functional changes introduced
- Simply propagating the the failure cases for the isAuAuMiniumBias flag.
@Steepspace
Update MinimumBiasClassifier - Suppress MB Check
3dd3441 
Suppressing the minimum bias check setting to false for total MBD charge above 2100.
@Steepspace
Update MinimumBiasClassifier - Adjust Defaults Loc
6530f15 
- Setting the defaults for the MinBias params at the top of the FillMinimumBiasInfo method to ensure they are set
@Steepspace
Propagate the MBD Information
b50c974 
- MBD Total Charge (North & South), vertex scale and centrality scale factor
@Steepspace
MinimumBiasClassifier - MBD Charge Thresh Var
f662eb1 
- Converted the total MBD Charge threshold from hard coded to into variable and swtiched to the new default of 2700
@Steepspace
Minimum Bias Params Refactor
f2a8004 
- Move the creation of minimum bias params outside of the GetNodes which
is called once per each event
- Only needs to be called once in the init
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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 and usage tips.

@sphenix-jenkins-ci
Copy link

sphenix-jenkins-ci bot commented Feb 10, 2026

Build & test report

Report for commit f2a80043c0be1260b531b261ab6d8d9886622872:
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

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Steepspace @pinkenburg

Comments