Copilot commented on code in PR #1257:
URL: https://github.com/apache/mahout/pull/1257#discussion_r3190755682
##########
qdp/qdp-python/benchmark/benchmark_throughput.py:
##########
@@ -170,6 +170,16 @@ def run_mahout(
return result.duration_sec, result.vectors_per_sec
+def _sample_dim(num_qubits: int, encoding_method: str) -> int:
+ if encoding_method == "basis":
+ return 1
+ if encoding_method in {"angle", "iqp-z"}:
+ return num_qubits
+ if encoding_method == "iqp":
+ return num_qubits + num_qubits * (num_qubits - 1) // 2
+ return 1 << num_qubits
Review Comment:
`basis` currently returns `1`, but this script’s benchmark utilities treat
the passed dimension as the *upper bound* for the basis index generation (i.e.,
should be `2**num_qubits`). With `1`, synthetic basis inputs will be all zeros,
producing incorrect/non-representative benchmarks. Fix by returning `1 <<
num_qubits` for `basis` here (and keep `angle/iqp/iqp-z` as parameter counts).
##########
qdp/qdp-python/benchmark/benchmark_latency.py:
##########
@@ -113,6 +113,16 @@ def run_mahout(
return result.duration_sec, result.latency_ms_per_vector
+def _sample_dim(num_qubits: int, encoding_method: str) -> int:
+ if encoding_method == "basis":
+ return 1
+ if encoding_method in {"angle", "iqp-z"}:
+ return num_qubits
+ if encoding_method == "iqp":
+ return num_qubits + num_qubits * (num_qubits - 1) // 2
+ return 1 << num_qubits
Review Comment:
Same issue as the throughput benchmark: returning `1` for `basis` causes
synthetic basis indices to be generated in `[0, 1)`, i.e., always `0`. Update
`basis` to return `1 << num_qubits` to preserve correct basis index ranges.
##########
qdp/qdp-python/benchmark/README.md:
##########
@@ -87,7 +87,7 @@ Notes:
- `--frameworks` is a comma-separated list or `all`.
Options: `mahout`, `pennylane`, `qiskit-init`, `qiskit-statevector`.
-- `--encoding-method` selects the encoding method: `amplitude` (default) or
`basis`.
+- `--encoding-method` selects the encoding method: `amplitude` (default),
`angle`, `basis`, `iqp`, or `iqp-z`.
- The latency test reports average milliseconds per vector.
- Flags:
- `--qubits`: controls vector length (`2^qubits`).
Review Comment:
This statement is no longer correct now that `--encoding-method` includes
`angle`, `iqp`, and `iqp-z` (where the input length is `num_qubits` or `n +
n(n-1)/2`). Update this line to describe that input length depends on the
selected encoding method (and specify the formulas).
##########
qdp/qdp-python/qumat_qdp/torch_ref.py:
##########
@@ -337,24 +337,30 @@ def encode(
encoding_method: str = "amplitude",
*,
device: torch.device | str | None = None,
- **kwargs: object,
+ enable_zz: bool = True,
) -> torch.Tensor:
"""Dispatch to the appropriate encoding function by method name.
Args:
data: Input tensor.
num_qubits: Number of qubits.
- encoding_method: One of ``"amplitude"``, ``"angle"``, ``"basis"``,
``"iqp"``.
+ encoding_method: One of ``"amplitude"``, ``"angle"``, ``"basis"``,
``"iqp"``, ``"iqp-z"``.
device: Target device.
- **kwargs: Extra arguments forwarded to the encoder (e.g. *enable_zz*
for IQP).
+ enable_zz: Whether IQP encoding includes ZZ interaction terms. Ignored
for
+ non-IQP encodings. ``"iqp-z"`` always forces this to ``False``.
Returns:
Complex tensor of shape ``(batch, 2**num_qubits)``.
"""
+ if encoding_method == "iqp-z":
+ return iqp_encode(data, num_qubits, device=device, enable_zz=False)
+ if encoding_method == "iqp":
+ return iqp_encode(data, num_qubits, device=device, enable_zz=enable_zz)
+
fn = _ENCODERS.get(encoding_method)
if fn is None:
raise ValueError(
f"Unknown encoding method {encoding_method!r}. "
f"Supported: {', '.join(sorted(_ENCODERS))}"
)
Review Comment:
The error message builds its supported-method list from `_ENCODERS`, but
`iqp-z` is handled via a special-case branch and may not appear in `_ENCODERS`.
This can produce a misleading 'Supported:' list. Consider including
special-cased methods (e.g., `iqp`/`iqp-z`) in the displayed supported set, or
building the list from a single source of truth.
##########
qdp/qdp-python/qumat_qdp/torch_ref.py:
##########
@@ -337,24 +337,30 @@ def encode(
encoding_method: str = "amplitude",
*,
device: torch.device | str | None = None,
- **kwargs: object,
+ enable_zz: bool = True,
) -> torch.Tensor:
Review Comment:
This change removes `**kwargs` from `encode()`, which can be a breaking API
change for any downstream callers passing extra encoder-specific options. If
backward compatibility is required, consider keeping `**kwargs` and either (a)
extracting `enable_zz` from it with a deprecation path, or (b) explicitly
rejecting unknown kwargs with a clear error message.
##########
qdp/qdp-core/src/pipeline_runner.rs:
##########
@@ -537,11 +537,13 @@ pub fn vector_len(num_qubits: u32, encoding_method: &str)
-> usize {
match encoding_method.to_lowercase().as_str() {
"angle" => n,
"basis" => 1,
+ "iqp-z" => n,
+ "iqp" => n + n.saturating_mul(n.saturating_sub(1)) / 2,
Review Comment:
The IQP length computation uses `saturating_mul`, but the final `n + (...)`
can still overflow `usize` for large `n` (wrapping in release, panicking in
debug depending on settings). Use `saturating_add` (or a checked add with a
clear error) to keep overflow behavior consistent with the rest of the
expression.
--
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]
