Skip to content

[QDP] Add CUDA kernel tests for basis encoding#1325

Open
tongyu0924 wants to merge 4 commits into
apache:mainfrom
tongyu0924:main
Open

[QDP] Add CUDA kernel tests for basis encoding#1325
tongyu0924 wants to merge 4 commits into
apache:mainfrom
tongyu0924:main

Conversation

@tongyu0924
Copy link
Copy Markdown

Changes

  • Test

Why

basis.cu had no test coverage.

How

Add tests/basis_encode.rs covering:

  • Single-sample encode: first, middle, last index (f64 + f32)
  • Batch encode: multiple samples with different indices (f64 + f32)
  • Error rejection: out-of-range index, zero state_len, zero num_samples
  • Non-Linux dummy stub

Checklist

  • Added or updated unit tests for all changes
  • Added or updated documentation for all changes

Comment thread qdp/qdp-kernels/tests/basis_encode.rs Outdated
let num_samples = 4usize;
let state_len = 4usize;
let num_qubits = 2u32;
let basis_indices: Vec<usize> = vec![0, 1, 2, 3];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have multiple test cases and hard testcase that test the limit and edge case of the code?

@400Ping
Copy link
Copy Markdown
Member

400Ping commented May 19, 2026

Please fix pre-commit

@400Ping 400Ping changed the title Add CUDA kernel tests for basis encoding [QDP] Add CUDA kernel tests for basis encoding May 19, 2026
Copy link
Copy Markdown
Member

@400Ping 400Ping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding kernel-level test coverage for basis encoding. I think this PR should be tightened in a few places:

  1. Please reduce the duplicated test setup/assertion logic.
    Most tests repeat CUDA device creation, allocation, launch, copy-back, and basis-state assertions. A small helper such as cuda_device_or_skip(), assert_basis_state_f64(), assert_basis_state_f32(), and table-driven cases for first/middle/last would make this much easier to maintain.

  2. Please add or clarify the batch-kernel edge cases.
    The batch kernels rely on state_len == 2^num_qubits because they compute:

    • sample_idx = global_idx >> num_qubits
    • element_idx = global_idx & (state_len - 1)

    Right now the launcher only rejects zero num_samples / state_len. If the intended contract is that callers must validate state_len, num_qubits, and basis-index bounds before calling the launcher, please document that in the test or PR description. Otherwise, please add validation/tests for:

    • batch out-of-range basis index
    • state_len / num_qubits mismatch
    • f32 batch zero state_len
  3. Please assert the imaginary part in the f32 batch test too.
    test_basis_encode_batch_f32_basic currently checks only actual.x, while the f64 batch test checks both real and imaginary values.

@400Ping 400Ping requested review from guan404ming and rich7420 May 25, 2026 19:09
@rich7420
Copy link
Copy Markdown
Contributor

rich7420 commented May 26, 2026

@tongyu0924 thanks for the patch!

nits: the state buffers are created with alloc_zeros, so the "all other elements should be 0" assertions pass even if the kernel never writes those slots — they're zero to begin with. Since basis encoding's contract is to overwrite the entire state vector, pre-filling the buffer with a sentinel (e.g. 1.0) before launch would actually verify the kernel zeroes them. Matters most for the batch kernels, where a grid-stride indexing bug could silently skip elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants