You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@mahout.apache.org by "viiccwen (via GitHub)" <gi...@apache.org> on 2026/03/15 03:35:46 UTC
[I] [Feature] Add tests for streaming basis encoder coverage [mahout]
viiccwen opened a new issue, #1182:
URL: https://github.com/apache/mahout/issues/1182
ref: #1058
### What
Add focused tests for `qdp/qdp-core/src/encoding/basis.rs`.
Current coverage for this file is still zero:
- Function coverage: `0.00% (0/6)`
- Line coverage: `0.00% (0/108)`
### Why
The streaming basis encoder contains CPU-side validation that should be stable and easy to verify, but none of it is covered right now.
That includes:
- invalid sample size handling
- negative and non-integer basis indices
- out-of-range basis indices
- the `needs_staging_copy() == false` path
### How
Add tests under `qdp/qdp-core/tests/` that exercise the basis streaming encoder directly.
Suggested coverage targets:
- assert `needs_staging_copy()` returns `false`
- reject `sample_size != 1`
- reject non-finite, negative, fractional, and out-of-range basis indices
- verify buffer reuse behavior across chunks if practical
- add one successful encode path for a small valid input
This issue is done when `encoding/basis.rs` is no longer completely uncovered in `cargo llvm-cov`.
--
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: issues-unsubscribe@mahout.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Feature] Add tests for streaming basis encoder coverage [mahout]
Posted by "viiccwen (via GitHub)" <gi...@apache.org>.
viiccwen closed issue #1182: [Feature] Add tests for streaming basis encoder coverage
URL: https://github.com/apache/mahout/issues/1182
--
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: issues-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Feature] Add tests for streaming basis encoder coverage [mahout]
Posted by "yehfelareborn (via GitHub)" <gi...@apache.org>.
yehfelareborn commented on issue #1182:
URL: https://github.com/apache/mahout/issues/1182#issuecomment-4062366021
take,I want to contribute this issue
--
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: issues-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Feature] Add tests for streaming basis encoder coverage [mahout]
Posted by "bearomorphism (via GitHub)" <gi...@apache.org>.
bearomorphism commented on issue #1182:
URL: https://github.com/apache/mahout/issues/1182#issuecomment-4414616452
This looks complete. The work landed in #1263 (merged 2026-04-20 by @yehfelareborn) — it just used `Ref #1182` instead of `Closes #1182`, so GitHub didn''t auto-close.
Mapping each acceptance bullet from the issue to where it''s satisfied on `main` (merge commit `95f475917`):
| Acceptance criterion | Where it lives |
| --- | --- |
| `encoding/basis.rs` no longer completely uncovered in `cargo llvm-cov` | PR description: 0.00% → **83.78% line / 71.43% function / 90.00% region** |
| assert `needs_staging_copy()` returns `false` | `needs_staging_copy_is_false` inline test in `qdp/qdp-core/src/encoding/basis.rs` |
| reject `sample_size != 1` | `test_basis_wrong_sample_size_rejected` |
| reject non-finite indices (NaN, Inf) | `test_basis_nan_rejected`, `test_basis_infinity_rejected` |
| reject negative indices | `test_basis_negative_rejected` |
| reject fractional indices | `test_basis_fractional_rejected` |
| reject out-of-range indices | `test_basis_out_of_range_rejected` |
| one successful encode path for a small valid input | `test_basis_successful_encoding_from_parquet` |
All seven integration tests live in `qdp/qdp-core/tests/gpu_basis_encoding.rs`.
Suggest closing this as done. Buffer-reuse-across-chunks was listed as "if practical" in the original ask and isn''t in #1263 — if anyone wants that specifically, it''d be a small follow-up issue rather than blocking this one.
--
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: issues-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Feature] Add tests for streaming basis encoder coverage [mahout]
Posted by "bearomorphism (via GitHub)" <gi...@apache.org>.
bearomorphism commented on issue #1182:
URL: https://github.com/apache/mahout/issues/1182#issuecomment-4414630937
Cross-checked the merged work against the file (`qdp-core/src/encoding/basis.rs` @ `52018d4`) for any future follow-up. **#1263 should still close this issue** — the acceptance criterion was met. Recording the remaining gaps so they don''t get rediscovered later.
### Biggest gap — kernel correctness on the happy path
`test_basis_successful_encoding_from_parquet` (`tests/gpu_basis_encoding.rs:170-193`) only calls `common::assert_dlpack_shape_2d_and_delete(dlpack_ptr, 4, 4)`, which checks `tensor.ndim == 2` and `shape == [4, 4]` (`tests/common/mod.rs:135-157`) — it never reads device data. So a basis kernel that returned a correctly-shaped but wrong statevector would still pass.
The 90% region / 84% line coverage looks great because every line of `encode_chunk` *runs*. But "runs" ≠ "verified" for the kernel side. Recommend: a follow-up that, for input `[i]` with `num_qubits = N`, asserts `state[i] == Complex(1.0, 0.0)` and all other amplitudes are zero. That single assertion would also retroactively cover the boundary indices `0` and `state_len - 1`, which are presently exercised but not asserted.
### Smaller gaps (cosmetic / low ROI)
| Gap | Where |
| --- | --- |
| All five rejection tests use a single-element bad input → `format!("Sample {}: ...", global_sample_offset + i, ...)` (`basis.rs:114-141`) is only ever rendered as `Sample 0:`. A test like `[0.0, 1.0, NaN]` would prove the index is reflected correctly. | `tests/gpu_basis_encoding.rs:58-166` |
| The validation loop''s partial-push-then-error path (`basis.rs:142` runs for valids before the early return on the first invalid) is never exercised — same fix as above. |
| Every basis test uses `num_qubits = 2`. Larger `num_qubits` would exercise kernel orchestration with realistic state sizes. | All tests |
| Validation order (`is_finite > non-negative > is_integer`) is not proven — `-1.0` is finite + integral + just negative, so it doesn''t exercise the order. Cosmetic. | `basis.rs:113-131` |
### Genuine non-gaps
| Apparent gap | Why it''s impractical |
| --- | --- |
| Multi-chunk buffer-reuse via `state.indices_cpu.clear()` | `STAGE_SIZE_ELEMENTS = 64M f64` (`encoding/mod.rs:69-70`), `pub(crate) const`, no `cfg(test)` override. Crossing a chunk boundary needs > 512 MB of input. The original issue correctly marked this "if practical." |
| `init_state` alloc failure (`basis.rs:77-82`), `htod_sync_copy_into` failure (`basis.rs:150-155`), kernel non-zero return (`basis.rs:168-173`) | No fault-injection infra in qdp-core (no `MockEngine`, no `cfg(test)` hook, no env-var gate — verified via repo-wide grep). These three are exactly the 2/7 uncovered functions in the 71.43% function-coverage figure. Not worth building a mock layer just for these. |
Net: ~1 high-value follow-up (state-contents verification), ~3 small cosmetic ones, ~3 not worth chasing.
--
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: issues-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org