[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.58% 71.60% +0.02%
- Complexity 49872 49897 +25
============================================
Files 1602 1602
Lines 193154 193157 +3
Branches 37817 37818 +1
============================================
+ Hits 138260 138308 +48
+ Misses 44106 44067 -39
+ Partials 10788 10782 -6 ☔ 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
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