[SYSTEMDS-3922] Fix federated quantile VALUEPICK averaging for even-length input - #2497
[SYSTEMDS-3922] Fix federated quantile VALUEPICK averaging for even-length input#2497MegaByteTron wants to merge 4 commits into
Conversation
|
Hi @MegaByteTron, please have a look at the following two commits |
|
Hi @ywcb00, thanks for the pointer! I've added a second commit ( The weighted branch ( Let me know if you'd like anything adjusted. |
4ace52d to
e64f3d0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2497 +/- ##
============================================
- Coverage 71.59% 71.58% -0.01%
+ Complexity 49893 49882 -11
============================================
Files 1602 1602
Lines 193154 193156 +2
Branches 37817 37820 +3
============================================
- Hits 138290 138275 -15
- Misses 44089 44099 +10
- Partials 10775 10782 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ength input
The federated QuantilePickFEDInstruction only enabled the even-length
averaging branch for MEDIAN, while the CP reference path
(QuantilePickCPInstruction) applies the same averaging convention for
VALUEPICK by passing matBlock.getLength()%2==0 to MatrixBlock.pickValue.
As a result, quantile(A, p) on a federated matrix with an even row count
diverged from the CP reference (which compareResults(1e-9) rejects),
causing the federatedQuantile{1,2,3}{CP,SP} tests to fail. They were
previously marked @ignore to hide the failure.
Extend the average flag to also cover VALUEPICK so the federated path
mirrors the CP averaging contract. The existing guard
average = average && (... rows ...) % 2 == 0
keeps averaging disabled for odd-length input, matching CP. IQM is
intentionally excluded because it has its own interpolation path.
Un-ignore the six previously-disabled tests.
The Spark path in QuantilePickSPInstruction.getWeightedQuantileSummary picked the value at ceil(p * n) without averaging with its right neighbor, diverging from the CP fix in 3ae6a77 that established even-length averaging as the SystemDS quantile convention. As a result the same DML program returned different values under CP vs SP for any matrix with an even row count (most visibly at p = 0.5). Mirror the CP condition: when the input length is even and the picked key is not the last row, add the next value and divide by two. This brings the unweighted SP path into agreement with CP for every quantile, not just the median. Also mirror the robustness guard from ebdabf1: the key < mc.getRows() check prevents an out-of-bounds lookup for boundary quantiles (p == 1.0 on even n). Un-ignore testQuantileBugSP, which is the SP analog of testQuantileBugCP and now passes on the same [1,5,7,10] median input. The weighted branch (mc.getCols() == 2) still uses the same non-averaged key lookup and remains flagged by the existing TODO for a later follow-up, matching the scope note in 3ae6a77.
The prior fix added even-length averaging inside getWeightedQuantileSummary unconditionally, which broke IQM because computeIQMCorrection uses the raw values at the Q25/Q75 positions as boundary values (wt[3], wt[4]) for the fractional-cell correction, not as picked values. Gate the averaging behind a new `average` flag: VALUEPICK and MEDIAN pass true (they consume wt[3] as the picked value), IQM passes false. Restores testIQM1_SP without regressing testQuantileBugSP or the FED tests.
…ode style
Pass the average flag through the ValuePick UDF (used by the column-
federated path) so single-shard VALUEPICK picks now average adjacent
order statistics for even-length inputs, matching CP semantics. Enables
the previously commented-out {1000, 1, false} case in FederatedQuantileTest.
Also simplifies the processRowQPick initialization so the parity guard
in the subsequent 'Average for median' step stays a straight AND, and
applies the Eclipse code-style fixes flagged by the Java Format Check
workflow on lines touched by this branch.
41a8b2c to
804ce5f
Compare
|
Thank you for this contribution @MegaByteTron :) |
Bring the FED path in line with the CP/SP switch to R quantile type 7 already on this branch. Row-federated matrices now return the same type-7 interpolated values as CP: for the ticket example [0.239, 0.517, 0.890, 0.944] at p = 0.25, 0.5, 0.75, output is 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 — a discriminator flag inside createHistogram regressed the pre-3953 attempt (isType7 on both getBucketWithIndex and createHistogram), 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. getSingleQuantileResult drops the IQM boolean too and is renamed / inlined where it was one-caller. IQM stays a raw ceil-based trimmed weighted mean and is extracted into private computeIqm, mirroring the SP computeIqm helper at QuantilePickSPInstruction.java:200 so the processRowQPick switch 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 on the branch), 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-unignored testQuartileArray {CP,SP}), IQMTest green, QuantilePickTest green.
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.
Summary
Two related fixes for the SystemDS quantile-pick averaging convention on
even-length inputs. The federated fix is the primary contribution
([SYSTEMDS-3922]); the Spark fix was added at reviewer request as the
SP analog of the CP work done in [SYSTEMDS-3898].
Commit 1 — [SYSTEMDS-3922] Federated VALUEPICK averaging
The federated
QuantilePickFEDInstructiononly enabled the even-lengthaveraging branch for
MEDIAN, while the CP reference path(
QuantilePickCPInstruction) applies the same averaging convention forVALUEPICKby passingmatBlock.getLength() % 2 == 0toMatrixBlock.pickValue.As a result,
quantile(A, p)on a federated matrix with an even rowcount diverged from the CP reference (which
compareResults(1e-9)rejects), causing the
federatedQuantile{1,2,3}{CP,SP}tests to fail.They were previously marked
@Ignoreto hide the failure.averageflag inprocessRowQPickto also coverVALUEPICK.IQMis intentionally excluded because it has its owninterpolation path.
(
federatedQuantile{1,2,3}{CP,SP}) and drop the unusedorg.junit.Ignoreimport.Commit 2 — [SYSTEMDS-3898] Spark VALUEPICK averaging
Added at David's (ywcb00) request in the PR discussion — the SP analog
of the CP work in 3ae6a77 and ebdabf1. Without it, running the same
DML program in CP vs SP mode returned different values on any
even-length input.
QuantilePickSPInstruction.getWeightedQuantileSummary:when the input length is even and the picked key is not the last row,
add the next value and divide by two.
key < mc.getRows()guard (SP analog of thepos < getNumRows() - 1fix in ebdabf1) for boundary quantiles.testQuantileBugSP.mc.getCols() == 2) still uses the same non-averagedkey lookup and remains flagged by the existing TODO — same scope note
as 3ae6a77.
Test plan
FederatedQuantileTest— all 12 tests pass locally (CP + SP),including the six previously-ignored ones.
federatedMedian{CP,SP},federatedIQR{CP,SP},federatedQuantiles{CP,SP}still pass —IQMandMEDIANpaths untouched.
QuantileTest— all 20+ tests pass locally, including thenewly-enabled
testQuantileBugSP.Resolves https://issues.apache.org/jira/browse/SYSTEMDS-3922
Related: https://issues.apache.org/jira/browse/SYSTEMDS-3898