Skip to content

feat: add pick command for reduce - #429

Open
henryiii wants to merge 7 commits into
boostorg:developfrom
henryiii:pick-reduce
Open

feat: add pick command for reduce#429
henryiii wants to merge 7 commits into
boostorg:developfrom
henryiii:pick-reduce

Conversation

@henryiii

@henryiii henryiii commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

This implements a pick command for algorithm::reduce, closes #275. Adds a new constructor:

Axis(const Axis& src, axis::pick_tag,
     const axis::index_type* begin, const axis::index_type* end)

To be pickable, an axis must have this constructor, using axis::pick_tag to opt in. Pointers are used in the signature to avoid forcing a specific collection like std::vector (and C++17's std::span is not available); this is similar to (It begin, It end) iterator-pair constructors of axis::variable and axis::category. Concrete pointers are used instead of a template so that axis::traits::is_pickable can detect the constructor with std::is_constructible. Categorical axes gain this new constructor.

Picking uses pick({2, 0}) (a std::vector). The items here do need to be a specific collection, similar to the std::initializer_list constructors of axis::variable and axis::category, because the type-erased reduce_command must store the indices in a concrete container.

🤖 AI text below 🤖

This adds a pick command to algorithm::reduce, which selects an arbitrary subset of bins by index from an axis which is not ordered, like the category axis. Unlike slice, the bins do not have to be adjacent, and the new axis keeps them in the order given.

auto h = make_histogram(axis::category<std::string>({"red", "green", "blue"}));
// ...
auto h2 = algorithm::reduce(h, pick({2, 0})); // axis is now {"blue", "red"}

Summary:

  • Counts in bins that were not picked go to the overflow bin, if present; otherwise they are dropped.
  • Axes opt in with a special constructor A(const A&, axis::pick_tag, const std::vector<index_type>&), detected by the new axis::traits::is_pickable trait. The pick_tag disambiguates the constructor from the generic (iterable, metadata) axis constructors, which could otherwise match by accident and break compilation of reduce for unrelated axes. Only category opts in.
  • pick cannot be combined with another reduce command on the same axis; indices must be unique and in range.

@henryiii
henryiii marked this pull request as draft July 23, 2026 13:15
Add a pick command to algorithm::reduce, which selects an arbitrary
subset of bins by index from an axis which is not ordered, like the
category axis. Unlike slice, the picked bins do not have to be
adjacent, and they may be given in any order, which reorders the bins
in the new axis. Counts in bins which are not picked are added to the
overflow bin, if it is present, consistent with how slice treats
unordered axes; otherwise they are discarded.

Axes opt into picking with a new special constructor which accepts the
original axis and a vector of bin indices to keep. The new trait
axis::traits::is_pickable detects this constructor, mirroring
is_reducible. axis::category implements it; ordered axes throw
invalid_argument, since picking a non-adjacent subset from them would
require changing the axis type, which reduce does not support yet.

Fixes boostorg#275

Assisted-by: ClaudeCode:claude-fable-5
Windows CI failed two ways:
- C1128 (too many sections) when compiling algorithm_reduce_test.cpp;
  add /bigobj for the test under CMake, matching the b2 build which
  already sets it globally and the existing fill/operators tests.
- C4702 (unreachable code, treated as error) for the non-pickable
  static_if branch in reduce(). Unlike is_reducible (true for all
  standard axes), is_pickable is false for most axes, so that branch is
  codegen'd and MSVC flags the value after the noreturn throw. Guard the
  function with a 4702 pragma, mirroring detail/fill.hpp.

Assisted-by: ClaudeCode:claude-opus-4.8
The is_pickable trait matched any axis whose generic (iterable, metadata)
constructor accepts a vector of indices, e.g. variable<double,
std::vector<int>>, which broke compilation of reduce() with any command
on such axes. The pick constructor now takes an axis::pick_tag so only
axes that opt in match the trait.

Also mirror the MSVC /bigobj flag in test/Jamfile, simplify the pick
branch of the fill loop, hoist index validation out of the static_if,
and copy metadata via metadata_base(src) like the shrink constructors.

Assisted-by: ClaudeCode:claude-fable-5
Replace the per-cell linear search over the picked indices with a
lookup table built once per axis. Picking 1000 of 2000 categories in a
2000x500 histogram drops from ~200 ms to ~15 ms, on par with slice.
The table also routes the underflow bin of a hypothetical pickable
axis to the overflow bin instead of reading out of bounds; the Axis
concept now documents this. Also move the MSVC pragma block above the
doc comment and clarify in the guide that pick cannot be combined
with other commands.

Assisted-by: ClaudeCode:claude-fable-5
pick accepts an optional slice_mode like slice. In crop mode the
counts of unpicked bins and of the original overflow bin are
discarded instead of being moved to the overflow bin.

Assisted-by: ClaudeCode:claude-fable-5
…tructor

The pick constructor was the only user-facing signature in the library
that requires std::vector. A pointer range matches the iterator-pair
idiom of the other axis constructors and the primitive arguments of the
reduce constructor, and does not freeze a container type into the Axis
concept.

Assisted-by: ClaudeCode:claude-fable-5
- Store the pick lookup table in a dedicated reduce_command member
  instead of overwriting the pick list mid-algorithm.
- Cross-reference the primary pick() overload docs instead of
  duplicating them.
- Drop the per-test /bigobj flag from the Jamfile; the project
  requirements already apply it to all MSVC tests.

Assisted-by: ClaudeCode:claude-fable-5
@henryiii

henryiii commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Testing this downstream, report below. Works great. The #423 change is known (we are accessing internals in fill, trivial update to match upstream). The 8x slower comment is known too, it's due to np.take's vectorized gather. It is something we could fix here, but it's not reduce specific; we could address this in a followup for all reductions (would need per-axis lut + bulk copies). I'm not sure how difficult that is, can investigate afterwards. The overflow bins now work, too, so the "experimental" warning can go away :).

🤖 AI text below 🤖

Verdict: yes, the feature works for us. The whole home-grown pick_set block in Histogram.__getitem__ (axis reconstruction + np.take copies, ~30 lines) collapsed into building _core.algorithm.pick(i, selection) commands that go through the same reduce() call as slices. All 1131 tests, mypy, and prek pass.

What I set up (all uncommitted, for the trial):

  • extern/histogram is on a local branch pick-on-1.90: the PR's 7 commits cherry-picked cleanly onto our pinned 1.90.0 commit. The PR branch itself is based on develop, which breaks our fill.hpp via an unrelated change (80fcf10, fix: avoid dangling-reference in sample tuple conversion for C++23 compatibility #423 sample-tuple conversion) — that's next-bump compat work, not a pick problem.
  • Bound pick in src/register_algorithm.cpp (both overloads, mirroring slice, plus __repr__ support) and added stubs.
  • Replaced the Python implementation in histogram.py:1479, updated tests and docs/user-guide/indexing.rst.

Feedback on the feature itself:

  1. The pick_tag opt-in design composes perfectly with our bindings. Our category axes are plain bh::axis::category instantiations, so they became pickable with zero changes on our side. Binding pick was mechanical.
  2. Semantics are an upgrade. Unpicked bins now land in overflow, so sum(flow=True) is preserved — exactly the gap that forced our "experimental" warning ("Removed bins are not placed in overflow"). I removed the warning and updated the docs. crop mode is available too, though the indexing API doesn't expose it yet. One old behavior is inexpressible with the new API (keep original overflow but drop unpicked bins — neither shrink nor crop), but nothing needs it.
  3. Minor behavior changes: duplicate indices (h[[0, 0]]) and empty lists now raise ValueError (previously silently allowed/weird). Metadata survives (__dict__ metadata test passes). I kept a Python pre-check so picking on ordered axes still raises RuntimeError rather than ValueError, preserving our API.
  4. The one real concern is performance at scale. Parity on the existing small benchmarks (~62 µs both ways), but on a 1000×500×50 histogram picking half the bins on two axes, reduce takes 320 ms vs 38 ms for the old vectorized np.take path — ~8× slower, because reduce walks every input bin individually through the per-bin command dispatch. Not blocking for typical categorical axes (tens of bins), but worth noting on the PR — a pick-only fast path or bulk copy per picked bin could close the gap.

So the PR can come out of draft as far as boost-histogram verification goes; the perf note is the one thing I'd consider addressing upstream first.

@henryiii
henryiii marked this pull request as ready for review August 4, 2026 01:11
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.

Add pick command for reduce

1 participant