[SYSTEMDS-3953] Switch quantile default to R quantile type 7 - #2585
Open
MegaByteTron wants to merge 3 commits into
Open
[SYSTEMDS-3953] Switch quantile default to R quantile type 7#2585MegaByteTron wants to merge 3 commits into
MegaByteTron wants to merge 3 commits into
Conversation
quantile(x, p) in SystemDS had two bugs at the kernel layer:
1. Scalar p vs matrix p returned different answers for the same input
because the average flag was not propagated from pickValues to
pickValue.
2. Even n averaged the two order statistics straddling ceil(p*n),
which matches R type 7 only at p = 0.5 and matches nothing named
in Hyndman and Fan 1996 elsewhere.
Switch the default to R quantile type 7 (h = (n-1)*p + 1, interpolate
between the two adjacent order statistics with weight g = h - floor(h)).
For the ticket example [0.239, 0.517, 0.890, 0.944] at p = 0.25, 0.5,
0.75 the kernel now returns 0.4475, 0.7035, 0.9035 — matching R.
MatrixBlock:
* computeType7Rank(long n, double p) is the new static helper — returns
{lo, hi, g} as double[3]. Sits next to computeIQMCorrection with the
same shape template. Every consumer (kernel, CP, SP, FED) picks
against this one primitive so the formula lives in exactly one place.
* pickUnweightedValue(double) picks against getNumRows() with type 7.
* pickWeightedValue(double) picks against Math.round(sumWeightForQuantile())
with the same formula, treating the two-column weighted input as an
expanded sorted sequence of length sumWeights.
* pickValue(double) is the new public API; the pickValue(double,
boolean average) overload is removed (dead switch — type 7 is
unconditional at the kernel layer).
* pickValues(qs, ret) drops the average flag.
* median() collapses to pickValue(0.5).
* interQuartileMean and computeIQMCorrection are untouched — IQM is a
trimmed weighted mean, a different statistic that does not fit the
type-7 interpolation shape.
CP: QuantilePickCPInstruction drops the matBlock.getLength() % 2 == 0
argument to pickValue and pickValues. median() call unchanged.
Tests:
* QuantilePickTest expected values updated to type-7 (odd-n cases
unchanged; even-n cases now interpolate).
* CompressedSortTest drops the pickValue(q, true) averaging line —
with the boolean overload gone, one assertion covers the new
unconditional-type-7 contract.
SP, FED, and integration tests come in follow-up commits on this branch.
Bring the Spark path in line with the kernel switch to R quantile type 7
in the previous commit. Row-partitioned single-column and weighted
two-column inputs both return type-7 interpolated values matching CP.
QuantilePickSPInstruction:
* processInstruction reads as a clean switch — VALUEPICK / MEDIAN go
through pickQuantileValues, IQM goes through the extracted
computeIqm. The pre-3953 shape mixed the two under a single
getWeightedQuantileSummary blob whose fields meant different things
per operation; the split makes each case's data shape explicit.
* pickQuantileValues handles N >= 1 quantiles uniformly. Weighted
branch stores rank g's in a primitive double[] and pulls (lo, hi, g)
triples from extractWeightedTriples; unweighted branch uses
MatrixBlock.computeType7Rank and interpolates from the collected
sorted values.
* computeIqm is the extracted IQM path — reuses extractWeightedTriples
but combines with computeIQMCorrection, not type 7. IQM behavior is
numerically unchanged from pre-3953.
* extractWeightedTriples is the shared helper. Locates the partition
holding each ceil-based key by scanning cumulative per-partition
weights, then invokes ExtractWeightedQuantileFunction to fetch the
(position, posPart, value) triples.
* Type7Rank inner class and getWeightedQuantileSummary are both
removed; the static kernel helper subsumes the former, and the
per-consumer split obsoletes the latter.
Tests: un-ignore testQuartileArrayCP / testQuartileArraySP (marked with
FIXME: fix SYSTEMDS-3953 by David); add QuantileEven{1,2,3}{CP,SP} at
n = 128 for p in {0.25, 0.5, 0.75}. Odd-n cases (rows = 1973) can't
exercise interpolation because the rank lands on an integer at classical
p; the even-n additions cover the g != 0 path in both CP and SP.
FED path in the follow-up commit on this branch.
Bring the FED path in line with the CP/SP switch to R quantile type 7 in the previous two commits. Row-federated matrices now return the same type-7 interpolated values as CP: [0.239, 0.517, 0.890, 0.944] at p = 0.25, 0.5, 0.75 gives 0.4475, 0.7035, 0.9035. processRowQPick's VALUEPICK / MEDIAN branch computes per-quantile rank triples (lo, hi, g) via MatrixBlock.computeType7Rank, flattens to a deduplicated int[] of ranks, feeds those to pickMultipleRanks (renamed from computeMultipleQuantiles to signal it now does one job: fetch the value at each rank), and interpolates (1 - g) * v_lo + g * v_hi at the caller. Decoupling ranks from interpolation lets the multi-rank pipeline stay type-7-agnostic — an earlier attempt threading an isType7 flag through createHistogram / getBucketWithIndex regressed VALUEPICK to type 1 and mis-computed q25 boundaries, so this shape avoids reintroducing that seam. Consequence of the type-7 unification: the boolean average thread across processRowQPick, createHistogram, getBucketWithIndex, and getSingleQuantileResult (which only ever handled the even-n median average case) is removed end-to-end. IQM stays a raw ceil-based trimmed weighted mean and is extracted into private computeIqm, mirroring the SP computeIqm helper from the previous commit so processRowQPick reads as VALUEPICK/MEDIAN -> rank pipeline, IQM -> computeIqm — symmetric with SP. refineBucket wraps the "recurse into a coarse-histogram bucket with a finer sub-histogram" step so computeIqm and pickMultipleRanks share the nextNumBuckets heuristic in one place instead of two. MEDIAN in processColumnQPick now dispatches through the VALUEPICK path with p = 0.5 (kernel pickValue(0.5) is already type 7 after the first commit), which lets the ColMedian inner UDF go away. VALUEPICK and its ValuePick UDF are otherwise unchanged. Weighted row-federated inherits the kernel's sum-of-weights extension for free: N = sumWeights in the two-column case flows through the same rank formula. Column-federated weighted stays out of scope, matching PR apache#2497. Verified: FederatedQuantileTest 24/24, FederatedQuantileWeightsTest 12/12, QuantileTest 32/32 (including newly un-ignored testQuartileArray{CP,SP} and the QuantileEven cases), IQMTest green, QuantilePickTest green.
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.
Summary
Fixes both issues reported in SYSTEMDS-3953:
scalar
pvs matrixpreturning different answers because theaverageflag was not propagated frompickValuestopickValue, andeven-
ninputs averaging the two order statistics straddlingceil(p·n)— which matches R type 7 only at
p = 0.5and matches nothing named inHyndman & Fan (1996) elsewhere.
Switches the SystemDS default to R quantile type 7 across the kernel,
CP, SP, and FED paths. Supersedes PR #2497
([SYSTEMDS-3922]/[SYSTEMDS-3898]) which patched averaging on even
nonlyfor
p = 0.5. Continues PR #2565by @ywcb00 which added the failing-tests-as-
@Ignorescaffolding(
d6aabf6867); those tests are un-ignored here.IQM is intentionally preserved — it is a trimmed weighted mean, not an
order-statistic pick, and does not fit the type-7 interpolation shape.
IQMTestis the guardrail.R type 7 (1-indexed,
n = |x|):Ticket example —
[0.239, 0.517, 0.890, 0.944]atp = 0.25, 0.5, 0.75now returns
0.4475, 0.7035, 0.9035, matching R.Commit 1 — [SYSTEMDS-3953] Switch quantile kernel to R quantile type 7
MatrixBlock.computeType7Rank(long n, double p)is the new statichelper returning
{lo, hi, g}— one primitive every consumer picksagainst so the formula lives in one place.
pickUnweightedValue/pickWeightedValue/pickValues/medianrewired to type 7;pickValue(double)becomes the public API and thepickValue(double, boolean average)overload is deleted.QuantilePickCPInstructiondrops the
matBlock.getLength() % 2 == 0argument.QuantilePickTestexpected values updated to type 7;
CompressedSortTestdrops thenow-dead
pickValue(q, true)averaging assertion.Commit 2 — [SYSTEMDS-3953] Rework Spark quantile pick to R type 7
processInstructionbecomes a clean switch: VALUEPICK / MEDIAN →pickQuantileValues, IQM → new privatecomputeIqm. The pre-3953shape mixed both under a
getWeightedQuantileSummaryblob whose fieldsmeant different things per operation.
pickQuantileValueshandlesN ≥ 1quantiles uniformly; weighted branch pulls(lo, hi, g)triplesfrom a new shared
extractWeightedTripleshelper.Type7Rankinnerclass and
getWeightedQuantileSummaryremoved.Un-ignore
testQuartileArray{CP,SP}(markedFIXME: fix SYSTEMDS-3953by David in
d6aabf6867) and addtestQuantileEven{1,2,3}{CP,SP}atn = 128— the odd-length default (n = 1973) can't exerciseg ≠ 0interpolation because at classical
pthe rank lands on an integer.Commit 3 — [SYSTEMDS-3953] Switch federated quantile pick to R type 7
processRowQPickcomputes per-quantile rank triples(lo, hi, g)upfront, flattens to a deduplicated
int[]of ranks, feeds those topickMultipleRanks(renamed fromcomputeMultipleQuantilesto signalits narrowed job), and interpolates at the caller. Decoupling ranks
from interpolation keeps the multi-rank pipeline type-7-agnostic.
Consequence: the
boolean averagethread across four signatures(
processRowQPick,createHistogram,getBucketWithIndex,getSingleQuantileResult) is removed end-to-end.computeIqmmirrorsthe SP helper from commit 2 so the two paths read symmetrically.
refineBucketshares the coarse-histogram refinement heuristic betweencomputeIqmandpickMultipleRanks. MEDIAN inprocessColumnQPickdispatches through VALUEPICK with
p = 0.5; theColMedianinner UDFgoes away.
Weighted row-federated inherits the kernel's sum-of-weights extension
(
N = Σw) for free. Column-federated weighted stays out of scope,matching PR #2497.