Copilot commented on code in PR #1100:
URL: https://github.com/apache/mahout/pull/1100#discussion_r2876181639
##########
qdp/qdp-python/src/pytorch.rs:
##########
@@ -182,19 +184,11 @@ 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', 'basis', 'iqp', or 'iqp-z' methods, got '{}'. \
+ "CUDA tensor encoding currently only supports {} methods, got
'{}'. \
Use tensor.cpu() to convert to CPU tensor for other encoding
methods.",
+ format_supported_cuda_encoding_methods(),
encoding_method
)));
Review Comment:
`format_supported_cuda_encoding_methods()` uses `CUDA_ENCODING_METHODS`, but
the actual supported-method logic is still hard-coded in the `match
method.as_str()` arms. That means the error message list and the validation
logic can drift (e.g., if someone adds a new match arm but forgets to update
`CUDA_ENCODING_METHODS`, or vice versa). Consider using
`CUDA_ENCODING_METHODS.contains(&method.as_str())` as the single gate for
“supported vs unsupported” (and make the match’s `_` arm
unreachable/internal-error), or add a unit test that asserts the constant list
matches the match arms.
##########
qdp/qdp-python/src/pytorch.rs:
##########
@@ -182,19 +184,11 @@ 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', 'basis', 'iqp', or 'iqp-z' methods, got '{}'. \
+ "CUDA tensor encoding currently only supports {} methods, got
'{}'. \
Use tensor.cpu() to convert to CPU tensor for other encoding
methods.",
+ format_supported_cuda_encoding_methods(),
encoding_method
)));
Review Comment:
This PR’s main behavior change is the content/formatting of the
unknown-CUDA-encoding error message, but there doesn’t appear to be a test
covering this error path. Since the repo already has CUDA validation tests,
consider adding a test that passes an unsupported `encoding_method` to
`engine.encode()` on a CUDA tensor and asserts the message includes the
supported-method list (or at least key method names). This will prevent future
regressions/drift.
--
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]
