Isso-W commented on PR #2542:
URL: https://github.com/apache/systemds/pull/2542#issuecomment-5099234981
Update: tests are now script-based
Thanks, that was a fair point. The parallel test was indeed a unit test in
disguise. It is removed and replaced with end-to-end tests that follow the
existing unique tests.
**What changed**
- Deleted `LibMatrixSketchUniqueParallelTest` (344 lines of direct
`LibMatrixSketch` calls).
- Added `uniqueCol.dml` and `UniqueCol`. The column-wise direction had no
script-level coverage at all before.
- Extended `UniqueBase` with two variants:
- `uniqueTestOrdered(...)` compares row by row instead of ignoring row
order. Usable where every row or column of the result holds a single value, so
the hash set iteration order cannot affect the outcome.
- `uniqueTestConstrainedMemory(...)` temporarily lowers
`InfrastructureAnalyzer.setLocalMaxMemory(...)` around the script run and
restores it in a `finally` block, following the pattern already used in
`ParForNaiveBayesTest` and `CompilerTestBase`.
- Added multi-threaded and sparse cases for all three directions. The sparse
inputs populate only every eighth row, every eighth column or every tenth cell,
which mirrors the dense/sparse split of the countDistinct tests.
24 cases in total, 22 of them run the DML script.
**Batched paths**
`RowCol` batching is now reachable end-to-end: `UniqueRowCol.testBatchedCP`
runs the script under a reduced budget and selects the batched path.
For `Row` and `Col` I did not manage to do the same. Those select batching
only when `numThreads * cellsPerIndex * 64` exceeds the budget, and reaching at
least two batches additionally requires `budget >= 2 * cellsPerIndex * 64`.
Both together put the budget into a `[2, numThreads)` window that depends on
the thread count of the executing machine, and it is empty for `numThreads =
2`. That is a consequence of the corrected guard: after it, `Row` and `Col`
only need batching when a single row or column is very large. So those two
paths are covered by a small unit test, `UniqueBatchedPathTest`, in the same
spirit as `CountDistinctRowOrColBase.testCPSparseLarge`, which also calls the
library directly to reach a state that DML cannot express.
Happy to drop that test if you would rather have the two paths uncovered.
**Verification**
I instrumented the path selection temporarily to confirm the tests reach
what they claim: the script runs use `k = 20`, the `RowCol` budget case selects
the batched path, and the sparse inputs are read in sparse format. All 24 cases
pass, Checkstyle reports 0 violations, and the format check is clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
