-
Notifications
You must be signed in to change notification settings - Fork 983
Enable CUDA tensor validation for iqp and iqp-z encodings #1093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,9 +182,18 @@ pub fn validate_cuda_tensor_for_encoding( | |
| ))); | ||
| } | ||
| } | ||
| "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 | ||
| ))); | ||
| } | ||
| } | ||
| _ => { | ||
| 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 '{}'. \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 with rich. should be dynamic.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opened an issue for it. cc @SuyashParmar
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryankert01 ok working on it |
||
| Use tensor.cpu() to convert to CPU tensor for other encoding methods.", | ||
| encoding_method | ||
| ))); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.