aloha1357 commented on PR #1387: URL: https://github.com/apache/mahout/pull/1387#issuecomment-4768750146
Thanks for the detailed review — these are exactly the right things to flag on this PR. **Context: intentional scaffolding** #1387 (PR2) is scoped as **TC-path scaffolding only**: `encode_batch_tc` API wiring, phase split (real/imag), and bank-conflict-free batch transpose. The full Hadamard / Kronecker / Tensor Core pipeline is delivered incrementally in the stacked PRs below, so each PR stays reviewable on its own. **Where your points are addressed** | Issue | PR2 (#1387) | Follow-up PR | |-------|-------------|--------------| | Q1 — TC API returns invalid IQP state (no FWT) | Scaffolding only (phase → recombine) | ** | Q2 — Full-ZZ batch reads wrong sample data (`data_len`) | Known limitation | **PR5** fixes | Q3 — Tests don't exercise `encode_batch_tc` | Tests call `engine.encode()` only | **PR5** | Q4 — Launcher hides CUDA alloc/launch failures | `return cudaSuccess` | **PR5** propagates | Q5 — Test quality (random data, precision-aware tolerance) | — | **PR5** normalization smo **Stack for review** - PR4: https://github.com/apache/mahout/pull/1391 - PR5: https://github.com/apache/mahout/pull/1390 - PR6: https://github.com/apache/mahout/pull/1389 **Question for you** If the PR2 scaffolding looks acceptable in principle, would you prefer to: 1. **Approve PR2 as scaffolding** and continue reviewing PR4 → PR5 → PR6 in order, or 2. **Hold final feedback until PR6/PR7**, where we will land the remaining Q4/Q5 fixes (cudaMalloc error propagation, deterministic/precision-aware tests)? Option 2 avoids repeatedly growing the same PR and keeps the review surface stable. Happy to adjust based on what works best for your review workflow. -- 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]
