Conversation
Collaborator
|
Error while checking build/O2/fullCI_slc9 for 0fa617e at 2026-10-05 14:51: Full log here. |
BinningPolicyBase took an ignoreOverflows flag. With it false, values outside the outermost edges of an axis got bins of their own instead of being mapped to -1 and dropped by groupTable(). Nothing used it. Across O2 and O2Physics the only callers passing false were five sites in test_ASoAHelpers.cxx, i.e. the test of the feature itself; no analysis ever selected it. For event mixing it is the wrong behaviour anyway, since it pairs collisions that the vertex or centrality cut excluded on purpose. The path was also subtly inconsistent: once one axis overflowed, the remaining axes restarted their edge search one index too high, so an underflow on a later axis was binned as that axis's first real bin. Drop the flag. getBin() keeps a single path, getOverflowShift() and mIgnoreOverflows go away, and getBinsCount() is just the edge count minus the dummy VARIABLE_WIDTH entry and the dropped out-of-range bin. If per-axis overflow bins are ever genuinely wanted, a BinningPolicyWithOverflow subclass is the way to add them back: a separate type cannot be confused with this one at a call site, which a boolean could. In the test, the two policies that differed only in the flag collapse into one, and the expectations lose the rows that fall outside the axes (2, 3, 5, 8 and 9 of testA). The surviving categories [0, 4, 7] and [1, 6] are unchanged, so the remaining tuples are exactly the old ones restricted to those rows. The whole o2-test-framework-core suite passes, 258 cases.
Collaborator
|
Error while checking build/O2/fullCI_slc9 for 67e83a5 at 2026-10-06 07:55: Full log here. |
This branch has not been deployed
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
2 participants
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.
BinningPolicyBase took an ignoreOverflows flag. With it false, values outside
the outermost edges of an axis got bins of their own instead of being mapped
to -1 and dropped by groupTable().
Nothing used it. Across O2 and O2Physics the only callers passing false were
five sites in test_ASoAHelpers.cxx, i.e. the test of the feature itself; no
analysis ever selected it. For event mixing it is the wrong behaviour anyway,
since it pairs collisions that the vertex or centrality cut excluded on
purpose. The path was also subtly inconsistent: once one axis overflowed, the
remaining axes restarted their edge search one index too high, so an underflow
on a later axis was binned as that axis's first real bin.
Drop the flag. getBin() keeps a single path, getOverflowShift() and
mIgnoreOverflows go away, and getBinsCount() is just the edge count minus the
dummy VARIABLE_WIDTH entry and the dropped out-of-range bin.
If per-axis overflow bins are ever genuinely wanted, a
BinningPolicyWithOverflow subclass is the way to add them back: a separate type
cannot be confused with this one at a call site, which a boolean could.
In the test, the two policies that differed only in the flag collapse into
one, and the expectations lose the rows that fall outside the axes (2, 3, 5, 8
and 9 of testA). The surviving categories [0, 4, 7] and [1, 6] are unchanged,
so the remaining tuples are exactly the old ones restricted to those rows. The
whole o2-test-framework-core suite passes, 258 cases.