[PWGCF] Add QA histograms - #17923
Merged
Merged
Conversation
fsii
requested review from
SwatiSaha-1997,
alibuild,
dsarkaralice,
isputows,
lauraser and
victor-gonzalez
as code owners
September 15, 2026 14:32
|
O2 linter results: ❌ 0 errors, |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Compatibility-breaking renames, changed MC defaults, and incomplete QA coverage must be addressed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds MC QA histograms to partNumFluc and refactors particle encoding and event configuration.
Changes:
- Adds collision-count and track-matching QA histograms.
- Converts particle encoding to compile-time species parameters.
- Renames configuration/output keys and changes MC selection defaults.
File summaries
| File | Summary |
|---|---|
PWGCF/EbyEFluctuations/Tasks/partNumFluc.cxx |
Adds MC QA and related refactoring. Moderate issues remain involving changed defaults, renamed public configuration and output keys, and QA coverage restricted by existing selections. |
Review details
Suppressed comments (4)
PWGCF/EbyEFluctuations/Tasks/partNumFluc.cxx:956
- The event-selection
Configurablekeys are also renamed (cfgBitsSelectionEvent,cfgFlagInelEvent, andcfgFlagInelEventMc). Existing workflow options using the old keys will no longer configure this task, so retain the old key strings or add a compatibility migration rather than changing the public interface in this QA addition.
Configurable<std::uint64_t> cfgBitsSelection{"cfgBitsSelection", std::uint64_t{0b00000000000001000000000000000000000000000000000000}, "Event selection bits"};
Configurable<bool> cfgFlagInel{"cfgFlagInel", true, "Flag of requiring INEL > 0"};
Configurable<bool> cfgFlagInelMc{"cfgFlagInelMc", true, "Flag of requiring MC INEL > 0"};
PWGCF/EbyEFluctuations/Tasks/partNumFluc.cxx:1218
- Changing this existing output object from
hNMcEventstohNEventsMcbreaks downstream QA scripts and macros that consume the counter, even though the counter's behavior is otherwise unchanged. Keep the old histogram key or provide a compatibility alias before renaming it.
hrCounter.add("hNEventsMc", ";;No. of MC Events", {HistType::kTH1D, {{NEs<McEventSelection>, -0.5, static_cast<double>(NEs<McEventSelection>) - 0.5, "Selection"}}});
PWGCF/EbyEFluctuations/Tasks/partNumFluc.cxx:956
- This changes the default of
cfgFlagInelMcfrom false to true, so existing MC jobs that do not set the option now reject collisions with!mcCollision.isInelGt0()and produce different yields, efficiencies, and QA distributions. Please keep the previous default or explicitly treat this as a separate, documented analysis-selection change.
Configurable<bool> cfgFlagInelMc{"cfgFlagInelMc", true, "Flag of requiring MC INEL > 0"};
PWGCF/EbyEFluctuations/Tasks/partNumFluc.cxx:2583
- This QA fill is reached only through
processMc, whosemcCollisionargument issoa::Filtered<aod::JoinedMcCollisions>withnumRecoCollision > 0(line 1002), and only after the INEL and MC-vertex rejection returns above. Thus the histogram omits zero-reconstruction collisions and all MC collisions failing those selections, so it cannot represent the requested collisions-per-MC-collision distribution. Fill it from the unfiltered MC-collision stream before these QA-independent cuts, or add a separate QA path.
hrQaMc.fill(C_CS("hNCollisionsPerMcCollision"), mcCollision.numRecoCollision());
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
isputows
approved these changes
Sep 16, 2026
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
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.
No description provided.