Skip to content

[SYSTEMDS-3922] Fix federated quantile VALUEPICK averaging for even-length input#2497

Open
MegaByteTron wants to merge 4 commits into
apache:mainfrom
MegaByteTron:SYSTEMDS-3922-federated-quantile-valuepick
Open

[SYSTEMDS-3922] Fix federated quantile VALUEPICK averaging for even-length input#2497
MegaByteTron wants to merge 4 commits into
apache:mainfrom
MegaByteTron:SYSTEMDS-3922-federated-quantile-valuepick

Conversation

@MegaByteTron

@MegaByteTron MegaByteTron commented Jun 21, 2026

Copy link
Copy Markdown

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 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 in processRowQPick to also cover
    VALUEPICK. IQM is intentionally excluded because it has its own
    interpolation path.
  • Un-ignore the six previously-disabled tests
    (federatedQuantile{1,2,3}{CP,SP}) and drop the unused
    org.junit.Ignore import.

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.

  • Mirror the CP condition in 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.
  • Add the key < mc.getRows() guard (SP analog of the
    pos < getNumRows() - 1 fix in ebdabf1) for boundary quantiles.
  • Un-ignore testQuantileBugSP.
  • Weighted branch (mc.getCols() == 2) still uses the same non-averaged
    key 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 — IQM and MEDIAN
    paths untouched.
  • QuantileTest — all 20+ tests pass locally, including the
    newly-enabled testQuantileBugSP.

Resolves https://issues.apache.org/jira/browse/SYSTEMDS-3922
Related: https://issues.apache.org/jira/browse/SYSTEMDS-3898

@ywcb00

ywcb00 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Hi @MegaByteTron, please have a look at the following two commits
3ae6a7764a
ebdabf15ba
, which fixed the respective mistake for CP execution. There are test cases named QuantileBug, which will trigger an error when implemented for FED and SP.
All the best,
David

@MegaByteTron

Copy link
Copy Markdown
Author

Hi @ywcb00, thanks for the pointer!

I've added a second commit (4ace52d) tagged [SYSTEMDS-3898] that mirrors the CP fix pattern for the unweighted SP path in QuantilePickSPInstruction.getWeightedQuantileSummary — the length-based mc.getRows() % 2 == 0 condition from 3ae6a77, plus the boundary guard analog of ebdabf1. testQuantileBugSP is un-ignored and green locally alongside the rest of QuantileTest.

The weighted branch (mc.getCols() == 2) is intentionally left as-is to match the scope note from 3ae6a77 about weighted kernels needing a more involved implementation — the existing TODO stays in place there.

Let me know if you'd like anything adjusted.

@MegaByteTron MegaByteTron force-pushed the SYSTEMDS-3922-federated-quantile-valuepick branch from 4ace52d to e64f3d0 Compare July 13, 2026 14:37
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.60%. Comparing base (895bbeb) to head (e64f3d0).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
.../instructions/spark/QuantilePickSPInstruction.java 80.00% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…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.
@MegaByteTron MegaByteTron force-pushed the SYSTEMDS-3922-federated-quantile-valuepick branch from 41a8b2c to 804ce5f Compare July 15, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants