Simplifications to Quantile Sketching#12033
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the CPU quantile sketching implementation by removing the specialized “approx-tree-only” sketch variant and consolidating usage around a single weighted quantile sketch type.
Changes:
- Remove
WXQSummary/WXQuantileSketchand replace templated sketch/template machinery with a concreteWQuantileSketch. - Introduce a standalone
QueueandWQSummaryContainerused byWQuantileSketch. - Make
SketchContainerImplnon-templated and update host/GPU call sites to use the unified sketch type.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/common/quantile.h |
Consolidates quantile sketch types into WQuantileSketch, introduces new Queue + WQSummaryContainer, updates sorted sketch container integration. |
src/common/quantile.cc |
Removes template instantiations and updates SketchContainerImpl implementations to the new unified sketch types. |
src/common/quantile.cuh |
Updates CUDA-side type aliasing to the new non-templated WQuantileSketch. |
src/common/hist_util.cu |
Updates LimitSizeLevel call site to use WQuantileSketch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit 3f55427.
| LOG(INFO) << " check quantile stats, nbig=" << nbig << ", n=" << n; | ||
| LOG(INFO) << " srcsize=" << src.size << ", maxsize=" << maxsize << ", range=" << range | ||
| << ", chunk=" << chunk; | ||
| src.Print(); |
There was a problem hiding this comment.
The Print function is no longer used after this PR, maybe remove the definition as well?
There was a problem hiding this comment.
Actually all of these are unused (!!):
WQSummary::MaxError
WQSummary::Query
WQSummary::MaxRank
WQSummary::MakeFromSorted
WQuantileSketch::PushSummary
WQSummary::Entry::CheckValid
WQSummary::CheckValid
WQuantileSketch::CheckValid
This is a first pass at reducing spaghetti code in quantile sketching. I have removed the version of sketch only used by the approx tree method. If the first is good enough for hist then it is good enough for approx.