Skip to content

Enable CUDA tensor validation for iqp and iqp-z encodings#1093

Open
SuyashParmar wants to merge 1 commit intoapache:mainfrom
SuyashParmar:fix/cuda-iqp-validation-clean
Open

Enable CUDA tensor validation for iqp and iqp-z encodings#1093
SuyashParmar wants to merge 1 commit intoapache:mainfrom
SuyashParmar:fix/cuda-iqp-validation-clean

Conversation

@SuyashParmar
Copy link
Contributor

QdpEngine.encode() supports iqp and iqp-z in qdp-core, but the CUDA tensor validation path in pytorch.rs currently rejects these methods and only allows amplitude, angle, and basis.

This creates inconsistent behavior:

CPU/list/NumPy paths can use iqp/iqp-z
CUDA tensor path fails early with an unsupported-method error
Expected behavior
CUDA tensor inputs should allow iqp and iqp-z when dtype/shape/device constraints are satisfied (same style as other supported encodings).

Copy link
Contributor

@viiccwen viiccwen left a comment

Choose a reason for hiding this comment

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

overall lg, leave some comments. : )
Edit: wait, the testing even not passed, pls MAKE SURE ur patches all correct.

Comment on lines +185 to +193
"iqp" | "iqp-z" => {
if !dtype_str_lower.contains("float64") {
return Err(PyRuntimeError::new_err(format!(
"CUDA tensor must have dtype float64 for {} encoding, got {}. \
Use tensor.to(torch.float64)",
method, dtype_str
)));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's duplicated with angle method block, could be merged together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah , agreed it can be merged with the angle path. Since this PR is already green and scoped to the behavior fix, I’m okay to keep this minimal and do that small refactor in a follow-up (unless you prefer I include it here). Also sounds good on edge-case coverage follow-up, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it should be refactor in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's happy path, I'll take a follow-up PR to cover edge case.

@ryankert01
Copy link
Member

not sure if he's on a gpu machine yet (:

@rich7420
Copy link
Contributor

locally passed tests.
A little thought:
but I think the unknown-encoding error now lists 'amplitude', 'angle', 'basis', 'iqp', or 'iqp-z' as the only CUDA-supported methods; if future encodings are added, this message in validate_cuda_tensor_for_encoding will need manual updates again—maybe we consider centralizing the supported method list to avoid drift.

@SuyashParmar
Copy link
Contributor Author

@ryankert01 @viiccwen @rich7420 Thanks everyone for the review and validation. CI is green, local validation was confirmed, and the CUDA iqp/iqp-z validation inconsistency is resolved.

_ => {
return Err(PyRuntimeError::new_err(format!(
"CUDA tensor encoding currently only supports 'amplitude', 'angle', or 'basis' methods, got '{}'. \
"CUDA tensor encoding currently only supports 'amplitude', 'angle', 'basis', 'iqp', or 'iqp-z' methods, got '{}'. \
Copy link
Member

Choose a reason for hiding this comment

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

+1 with rich. should be dynamic.

Copy link
Member

Choose a reason for hiding this comment

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

opened an issue for it. cc @SuyashParmar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryankert01 ok working on it

Copy link
Member

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

lgtm

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