Add GNN-Based Predictor with DAG Preprocessing #430
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| TYPE_CHECKING = False | ||
| if TYPE_CHECKING: | ||
| VERSION_TUPLE = tuple[int | str, ...] |
Check warning
Code scanning / CodeQL
Unreachable code Warning
…into gnn-branch
…into gnn-branch
…into gnn-branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/hellinger_distance/test_estimated_hellinger_distance.py (1)
308-308: Use explicit if/else instead of ternary for side-effect-only calls.The ternary expression
ml_predictor.train_gnn_model() if gnn else ml_predictor.train_random_forest_model()is used purely for side effects (return value discarded), making the intent less clear than an explicit conditional block.Apply this diff for better readability:
- ml_predictor.train_gnn_model() if gnn else ml_predictor.train_random_forest_model() + if gnn: + ml_predictor.train_gnn_model() + else: + ml_predictor.train_random_forest_model()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/hellinger_distance/test_estimated_hellinger_distance.py(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
🪛 Ruff (0.14.8)
tests/hellinger_distance/test_estimated_hellinger_distance.py
42-42: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (2)
tests/hellinger_distance/test_estimated_hellinger_distance.py (2)
168-231: LGTM! Well-structured parametrized test for both RF and GNN paths.The test appropriately:
- Skips GNN tests when dependencies are unavailable
- Constructs correct training data formats for each model type (feature vectors for RF, Data objects for GNN)
- Passes
num_nodesas an integer (not a tensor)- Converts labels to numpy array for type safety
- Documents the intentionally loose GNN tolerance (5e-1) as a smoke test given limited training samples
290-298: Dataset assertions appropriately validate GNN output structure.The assertions correctly verify:
- Dataset directory exists and is a directory
- At least one file is present with
.safetensorsextension- File stems are numeric (as expected per predictor implementation)
This adequately validates the GNN dataset creation without being overly restrictive.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/mqt/predictor/ml/helper.py (1)
562-595: Consider avoiding duplicate verbose output.When
verbose=True, the evaluation functions print their own metrics (e.g., line 493 inevaluate_regression_model), and thentrain_modelprints an epoch summary (lines 583-595). This results in redundant output per epoch. Consider passingverbose=Falseto the evaluation calls and only using the epoch summary:if task == "classification": val_loss, val_metrics, _ = evaluate_classification_model( - model, val_loader, loss_fn, device=str(device), verbose=verbose + model, val_loader, loss_fn, device=str(device), verbose=False ) elif task == "regression": val_loss, val_metrics, _ = evaluate_regression_model( - model, val_loader, loss_fn, device=str(device), verbose=verbose + model, val_loader, loss_fn, device=str(device), verbose=False )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/predictor/ml/helper.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
🧬 Code graph analysis (1)
src/mqt/predictor/ml/helper.py (1)
src/mqt/predictor/utils.py (1)
calc_supermarq_features(94-146)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (8)
src/mqt/predictor/ml/helper.py (8)
18-34: LGTM!The imports are well-organized with appropriate separation between runtime and type-checking imports.
53-58: LGTM!The function correctly returns the path to the GNN checkpoint file with a clear docstring and proper f-string usage.
119-152: LGTM!The gate list is well-documented with a reference to the OpenQASM 3.0 specification.
314-326: LGTM!The feature dimension calculation is well-documented and correctly matches the feature construction in
create_dag.
329-347: LGTM!The function now correctly handles all target encodings (1D,
[N, 1], and one-hot) as suggested in previous reviews.
353-432: LGTM!The classification evaluator is well-structured with proper empty loader handling, shape validation, and comprehensive metrics computation. The static analysis warning about
arraysat line 409 is a false positive—arraysis properly assigned on line 408 before the return statement.
435-496: LGTM!The regression evaluator correctly handles empty loaders and edge cases in R2 calculation.
610-621: LGTM!The
TrainingDatadataclass is correctly typed to support both classical (numpy) and GNN (graph) data pipelines with a clear docstring.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/predictor/ml/helper.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
🧬 Code graph analysis (1)
src/mqt/predictor/ml/helper.py (1)
src/mqt/predictor/utils.py (1)
calc_supermarq_features(94-146)
🔇 Additional comments (2)
src/mqt/predictor/ml/helper.py (2)
312-324: LGTM! Feature dimension calculation is correct.The function correctly computes the total feature dimension (40) matching the concatenation in
create_dag. The docstring clearly explains each component.
560-578: LGTM! Validation logic is correct.The validation evaluation correctly dispatches based on
tasktype, andval_loss/val_metricsare always initialized before use within theif val_loader is not None:block. The static analysis warnings about uninitialized variables appear to be false positives—theValueErrorraised on line 578 ensures execution stops iftaskis invalid, preventing any uninitialized access.
Signed-off-by: Antonio Tudisco <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(4 hunks)src/mqt/predictor/ml/helper.py(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
📚 Learning: 2025-12-13T20:08:45.549Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 862
File: pyproject.toml:65-66
Timestamp: 2025-12-13T20:08:45.549Z
Learning: In the qmap project (pyproject.toml), maintain broad compatibility with dependencies across supported Python versions. Avoid artificially raising minimum version requirements unless there's a specific need (e.g., to guarantee binary wheel availability for certain Python versions, or to access required features). The goal is to keep the software as broadly compatible as possible with the rest of the ecosystem.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-27T21:22:11.330Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 846
File: pyproject.toml:213-214
Timestamp: 2025-11-27T21:22:11.330Z
Learning: In the qmap project (pyproject.toml), the maintainers intentionally enable `unsafe-fixes = true` in the ruff configuration. This is an established practice that has worked well for the project, with the expectation that PR authors review any automated fixes.
Applied to files:
pyproject.toml
📚 Learning: 2025-12-15T01:59:17.023Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: python/mqt/core/ir/operations.pyi:9-16
Timestamp: 2025-12-15T01:59:17.023Z
Learning: In the munich-quantum-toolkit/core repository, stub files (.pyi) are auto-generated by nanobind's stubgen tool and should not be manually modified for style preferences, as changes would be overwritten during regeneration.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (9)
pyproject.toml (2)
124-125: LGTM on warning filter specificity.The deprecation warning filters are now appropriately specific:
- Line 124 targets the exact
torch_geometric.distributeddeprecation- Line 125 targets the specific
typing._eval_typeparameter deprecationThese narrow filters follow best practices and were properly refined from earlier broader patterns.
175-180: LGTM on mypy override with clear documentation.The mypy override for the GNN module is properly documented and appropriately scoped:
- The comment clearly explains that torch and torch_geometric have incomplete type stubs
- The override only applies to
mqt.predictor.ml.gnn, not the entire ML module- The relaxation of
disallow_subclassing_anyis necessary for working withAny-typed returns from these librariesThis follows best practices for handling external libraries with incomplete typing.
src/mqt/predictor/ml/helper.py (7)
53-57: LGTM on GNN path helper.The function correctly:
- Returns a path to a GNN checkpoint file (not a folder)
- Uses an f-string for clear path construction
- Has an accurate, concise docstring
This was properly refined in previous reviews.
119-151: LGTM on OpenQASM 3.0 gate enumeration.The function provides a well-documented list of OpenQASM 3.0 standard gates:
- Includes a reference URL to the OpenQASM specification
- Documents the spec version (3.0) for future maintenance
- Covers the standard gate set needed for GNN-based circuit analysis
192-309: LGTM on comprehensive DAG feature extraction.The
create_dagfunction is well-structured and handles edge cases properly:
- Removes barriers and transpiles to OpenQASM 3.0 basis with error handling (lines 209-216)
- Uses defensive
_safe_floathelper to handle symbolic parameters without crashing (lines 222-226)- Computes rich node features: one-hot gate encoding, sin/cos parameters, arity, controls, num_params, critical path flags, and fan-in/fan-out
- Handles empty DAGs, missing edges, and nodes not in topological order with defensive
.get()calls- Returns properly shaped tensors for GNN consumption
Previous reviews thoroughly refined this implementation.
327-345: LGTM on classification evaluation utilities.Both functions are robust and well-designed:
get_results_classes(lines 327-345):
- Correctly handles one-hot encoded targets (2D) and integer class labels (1D or [N,1])
- Uses conditional logic to avoid
argmaxcorruption of integer labels
evaluate_classification_model(lines 351-430):
- Guards against empty loaders to prevent
torch.catcrashes (lines 405-407)- Properly computes accuracy, classification report, and optional regression metrics
- Return type correctly reflects both numeric and string metrics
- Handles shape mismatches with clear error messages
Previous reviews thoroughly refined these implementations.
Also applies to: 351-430
433-494: LGTM on regression evaluation function.The function correctly implements regression model evaluation:
- Properly aggregates loss across batches
- Handles empty loaders safely with concatenation guards (lines 480-481)
- Computes RMSE, MAE, and R2 with appropriate edge case handling
- Returns NaN for R2 when targets are constant or sample size < 2 (line 487)
- Respects the verbose flag for optional output
The implementation is clean and defensive.
497-611: LGTM on training function with early stopping.The
train_modelfunction implements a solid training loop:
- Supports both classification and regression tasks via the
taskparameter- Implements proper early stopping with
patienceandmin_deltathresholds- Restores best model weights when
restore_best=True(lines 609-611)- Passes
verboseflag through to evaluation functions (lines 566, 573)- Displays task-appropriate metrics in verbose output: accuracy for classification, R² for regression (lines 587-599)
- Handles device placement correctly throughout
Previous reviews refined the verbose output and metric display logic.
614-625: LGTM on TrainingData dataclass update.The dataclass correctly supports both classical and GNN models:
- Clear docstring explains it containers data for both numpy-based and graph-based models
- Uses appropriate union types:
NDArray[np.float64] | list[torch_geometric.data.Data]for features- Supports both numpy arrays and torch Tensors for labels
- Maintains backward compatibility while enabling GNN workflows
This provides a clean, type-safe container for the dual training paths.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-13T20:08:45.549Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 862
File: pyproject.toml:65-66
Timestamp: 2025-12-13T20:08:45.549Z
Learning: In the qmap project (pyproject.toml), maintain broad compatibility with dependencies across supported Python versions. Avoid artificially raising minimum version requirements unless there's a specific need (e.g., to guarantee binary wheel availability for certain Python versions, or to access required features). The goal is to keep the software as broadly compatible as possible with the rest of the ecosystem.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-27T21:22:11.330Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 846
File: pyproject.toml:213-214
Timestamp: 2025-11-27T21:22:11.330Z
Learning: In the qmap project (pyproject.toml), the maintainers intentionally enable `unsafe-fixes = true` in the ruff configuration. This is an established practice that has worked well for the project, with the expectation that PR authors review any automated fixes.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-04T15:22:19.558Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:155-157
Timestamp: 2025-11-04T15:22:19.558Z
Learning: The munich-quantum-toolkit/core repository requires Python 3.10 or later, so Python 3.10+ features (such as `zip(..., strict=...)`, pattern matching, etc.) are acceptable and should not be flagged as compatibility issues.
Applied to files:
pyproject.toml
📚 Learning: 2025-12-15T01:59:17.023Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: python/mqt/core/ir/operations.pyi:9-16
Timestamp: 2025-12-15T01:59:17.023Z
Learning: In the munich-quantum-toolkit/core repository, stub files (.pyi) are auto-generated by nanobind's stubgen tool and should not be manually modified for style preferences, as changes would be overwritten during regeneration.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (4)
pyproject.toml (4)
123-124: LGTM: Warning filters are appropriately specific.Both filters target specific deprecation warnings:
- Line 123 suppresses the known
torch_geometric.distributeddeprecation- Line 124 handles the
typing._eval_typeparameter deprecationThese are appropriately narrow and won't mask unexpected warnings.
171-171: LGTM: Mypy overrides appropriately extended for new dependencies.The additions of
torch,torch_geometric,optuna, andsafetensorsmodules to theignore_missing_importsoverride are necessary because these libraries lack complete type stubs, as documented in the comment above.
174-179: LGTM: Mypy override for GNN module is well-documented and appropriately scoped.The override is narrowly targeted to
mqt.predictor.ml.gnnwhere interaction with torch/torch_geometric'sAny-typed APIs is necessary, and includes a clear explanatory comment as requested in previous reviews.
267-267: LGTM: Appropriate typos dictionary entry for TPE acronym.Adding "TPE" (Tree-structured Parzen Estimator) to the typos dictionary is correct, as this is a valid acronym used in Optuna's hyperparameter optimization context.
| "numpy>=1.22", | ||
| "optuna>=4.0.0", | ||
| "torch-geometric>=2.7.0,<2.8.0", | ||
| "torch>=2.7.0,<2.8.0; sys_platform == 'darwin' and 'x86_64' in platform_machine and python_version < '3.13'", # Restrict torch v2.3.0 for macOS x86 since it is not supported anymore. |
There was a problem hiding this comment.
Update stale comment about torch version.
The comment states "Restrict torch v2.3.0 for macOS x86" but the actual constraint is torch>=2.7.0,<2.8.0. The comment should reflect the current version to avoid confusion.
Apply this diff to correct the comment:
- "torch>=2.7.0,<2.8.0; sys_platform == 'darwin' and 'x86_64' in platform_machine and python_version < '3.13'", # Restrict torch v2.3.0 for macOS x86 since it is not supported anymore.
+ "torch>=2.7.0,<2.8.0; sys_platform == 'darwin' and 'x86_64' in platform_machine and python_version < '3.13'", # Restrict torch to 2.7.x for macOS x86 since 2.8.0+ is not supported on this platform.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "torch>=2.7.0,<2.8.0; sys_platform == 'darwin' and 'x86_64' in platform_machine and python_version < '3.13'", # Restrict torch v2.3.0 for macOS x86 since it is not supported anymore. | |
| "torch>=2.7.0,<2.8.0; sys_platform == 'darwin' and 'x86_64' in platform_machine and python_version < '3.13'", # Restrict torch to 2.7.x for macOS x86 since 2.8.0+ is not supported on this platform. |
🤖 Prompt for AI Agents
In pyproject.toml around line 49, the inline comment incorrectly references
"Restrict torch v2.3.0 for macOS x86" while the actual constraint is
torch>=2.7.0,<2.8.0; update the comment to accurately reflect the current
constraint (e.g., "Restrict torch >=2.7.0,<2.8.0 for macOS x86 since older
versions are not supported") so the comment and the version specifier match.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/hellinger_distance/test_estimated_hellinger_distance.py:
- Around line 298-299: The inline comment above the assertion incorrectly says
".safetensor" (singular) and has a missing space in "asdefined"; update the
comment to match the actual check by changing ".safetensor" to ".safetensors"
and fix spacing/typo (e.g., "as defined in the predictor") so the comment
accurately describes the assertion that files end with ".safetensors" and their
stem is numeric.
- Around line 168-173: Rename the test parameter to avoid the boolean-name lint
warning: update the parametrize tuple from ("model_type", "verbose") to
("model_type", "verbose_flag") and change the test function signature from
test_train_model_and_predict(..., verbose: bool) to verbose_flag: bool; then
replace uses of verbose inside the function with verbose_flag (e.g., where the
flag controls verbosity or is passed into helpers). Keep the existing
model_type/gnn logic (the gnn variable and _HAS_GNN_DEPS check) intact.
- Around line 237-242: The test function
test_train_and_qcompile_with_hellinger_model is missing the GNN dependency guard
and will fail when model_type == "gnn" but torch/torch_geometric are not
installed; add the same early skip used in test_train_model_and_predict: if gnn
and (missing torch or torch_geometric), call pytest.skip with a clear message
before any GNN-specific logic. Locate
test_train_and_qcompile_with_hellinger_model and insert the dependency
check/pytest.skip (reusing the same import checks or helper used in the other
test) so GNN-parameterized runs are skipped when prerequisites are absent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/hellinger_distance/test_estimated_hellinger_distance.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-25T13:28:25.619Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:25.619Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with an extensive set of rules including SLF (flake8-self) in the extend-select list. The `# noqa: SLF001` directive for private member access (e.g., `self.state._layout = self.layout` in src/mqt/predictor/rl/predictorenv.py) is necessary and appropriate, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
🧬 Code graph analysis (1)
tests/hellinger_distance/test_estimated_hellinger_distance.py (3)
src/mqt/predictor/ml/helper.py (3)
TrainingData(615-625)create_dag(192-309)get_path_training_data(36-38)src/mqt/predictor/rl/helper.py (1)
get_path_training_data(97-99)src/mqt/predictor/hellinger/utils.py (1)
calc_device_specific_features(37-132)
🪛 Ruff (0.14.10)
tests/hellinger_distance/test_estimated_hellinger_distance.py
169-169: Boolean-typed positional argument in function definition
(FBT001)
238-238: Boolean-typed positional argument in function definition
(FBT001)
🔇 Additional comments (5)
tests/hellinger_distance/test_estimated_hellinger_distance.py (5)
37-46: LGTM!The optional GNN dependency handling is well-structured with proper
ImportErrornarrowing and fallback assignments for type checking.
61-64: LGTM!The
devicefixture now correctly declares its return type asTarget.
274-286: Verify: Windows timeout warning exclusion for GNN path.The comment indicates the timeout warning on Windows applies "just for RF model". If the GNN path uses the same
compile_training_circuitsmethod withtimeout=600, please verify this exclusion is intentional and that the GNN codepath genuinely doesn't trigger this warning on Windows.
342-389: LGTM!The cleanup fixture is well-designed:
- Uses the proper
yieldpattern for module-scoped teardown- Includes clear documentation explaining the conservative cleanup approach
- Properly handles nested directories in
graph_dataset_*folders- Uses tuple membership for suffix checks (
.npy,.pt,.safetensors, etc.)
321-339: LGTM!The error handling test correctly validates that providing multiple devices for Hellinger distance training raises the expected
ValueError.
Signed-off-by: Antonio Tudisco <[email protected]>
…into gnn-branch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/mqt/predictor/_version.py:
- Around line 21-27: Replace the hardcoded "TYPE_CHECKING = False" assignment
with a proper import from typing (use "from typing import TYPE_CHECKING") so the
conditional block becomes reachable during static type checking; keep the
existing TYPE_CHECKING conditional that defines VERSION_TUPLE and COMMIT_ID for
type checkers and the else branch that assigns object to VERSION_TUPLE and
COMMIT_ID.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/mqt/predictor/_version.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T13:28:19.850Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:19.850Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff has a broad rule set including FLake8-SLF (slf) in extend-select. For private member access (e.g., self.state._layout = self.layout) in src/mqt/predictor/rl/predictorenv.py, include a per-file noqa directive: # noqa: SLF001. This is appropriate even if Ruff reports RUF100 as unused, to acknowledge intentional private attribute access and to avoid false positives in this specific code path. Apply this directive only to the files where private-member access is intentionally used and where SLF001 is the correct rationale.
Applied to files:
src/mqt/predictor/_version.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (1)
src/mqt/predictor/_version.py (1)
1-10: Remove this generated file from version control.This file is explicitly marked as generated by setuptools-scm and should not be tracked in version control (lines 9-10). Tracking generated files causes merge conflicts and synchronization issues. The file will be automatically generated during package installation from your git metadata.
Add
src/mqt/predictor/_version.pyto.gitignoreand remove it from the repository.⛔ Skipped due to learnings
Learnt from: denialhaag Repo: munich-quantum-toolkit/core PR: 1383 File: python/mqt/core/ir/operations.pyi:9-16 Timestamp: 2025-12-15T01:59:17.023Z Learning: In the munich-quantum-toolkit/core repository, stub files (.pyi) are auto-generated by nanobind's stubgen tool and should not be manually modified for style preferences, as changes would be overwritten during regeneration.
| TYPE_CHECKING = False | ||
| if TYPE_CHECKING: | ||
| VERSION_TUPLE = tuple[int | str, ...] | ||
| COMMIT_ID = str | None | ||
| else: | ||
| VERSION_TUPLE = object | ||
| COMMIT_ID = object |
There was a problem hiding this comment.
Unreachable code in TYPE_CHECKING block.
Line 21 sets TYPE_CHECKING = False, making lines 23-24 unreachable. The standard practice is to import TYPE_CHECKING from the typing module:
from typing import TYPE_CHECKINGHowever, since this is a generated file that should not be tracked in version control, this issue becomes moot once the file is removed from the repository.
🤖 Prompt for AI Agents
In @src/mqt/predictor/_version.py around lines 21 - 27, Replace the hardcoded
"TYPE_CHECKING = False" assignment with a proper import from typing (use "from
typing import TYPE_CHECKING") so the conditional block becomes reachable during
static type checking; keep the existing TYPE_CHECKING conditional that defines
VERSION_TUPLE and COMMIT_ID for type checkers and the else branch that assigns
object to VERSION_TUPLE and COMMIT_ID.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/mqt/predictor/ml/predictor.py:
- Around line 544-546: Remove the per-batch debug print statements in the
training loop of src/mqt/predictor/ml/predictor.py (look inside the
train/train_epoch function or the loop that constructs the model with
output_dim=num_outputs, dropout_p=dropout, bidirectional=bidirectional); delete
any print(...) calls that run every batch or change them to conditional logging
(e.g., logger.debug behind a verbose flag) or aggregate and log once per epoch
instead.
- Around line 527-536: Remove the leftover debug print statements in the
cross-validation loop: delete the prints that output "Predictor", "Train
subset:", the loop that prints each batch from train_loader, and the prints that
show batch_device.y and "preds:"/targets shapes; if you need runtime visibility
replace them with proper logging at debug level (e.g., logger.debug) tied to the
training function that builds train_loader/val_loader so normal stdout is not
polluted. Ensure references to DataLoader, train_loader, val_loader,
batch_device, preds and targets remain unchanged except for removing the print
calls.
In @tests/compilation/test_predictor_rl.py:
- Around line 112-148: The two tests
test_qcompile_with_newly_trained_models_quantinuum and
test_qcompile_with_newly_trained_models duplicate setup/assertions; refactor by
parameterizing the device name (e.g., via pytest.mark.parametrize with values
"ibm_falcon_127" and "quantinuum_h2_56") or extract a small helper that accepts
device_name and runs the common sequence (get_device, get_benchmark,
Predictor(...), model_path check + pytest.raises, Predictor.train_model,
rl_compile, GatesInBasis checks and final asserts). Keep unique expectations
(the FileNotFoundError match) computed from the device/figure_of_merit so
rl_compile, Predictor.train_model, get_device, get_benchmark, and GatesInBasis
references remain clear and reused.
In @tests/device_selection/test_predictor_ml.py:
- Line 131: The assertion using two equality checks should be simplified to use
membership testing; replace the expression comparing predicted.description with
"ibm_falcon_127" or "quantinuum_h2_56" (the assert in
tests/device_selection/test_predictor_ml.py referencing predicted.description)
with an assertion that predicted.description is in the list or tuple of allowed
values to make the test clearer and more idiomatic.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.pytests/compilation/test_predictor_rl.pytests/device_selection/test_predictor_ml.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-25T13:28:10.402Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:10.402Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with the "SLF" (flake8-self) rule category enabled in extend-select. SLF001 (private member access) is active, so `# noqa: SLF001` directives are necessary when accessing private methods/attributes (e.g., `_ensure_device_averages_cached()`), even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-25T13:28:19.850Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:19.850Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff has a broad rule set including FLake8-SLF (slf) in extend-select. For private member access (e.g., self.state._layout = self.layout) in src/mqt/predictor/rl/predictorenv.py, include a per-file noqa directive: # noqa: SLF001. This is appropriate even if Ruff reports RUF100 as unused, to acknowledge intentional private attribute access and to avoid false positives in this specific code path. Apply this directive only to the files where private-member access is intentionally used and where SLF001 is the correct rationale.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/device_selection/test_predictor_ml.pytests/compilation/test_predictor_rl.py
📚 Learning: 2025-12-21T22:35:08.572Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/fomac/fomac.cpp:348-364
Timestamp: 2025-12-21T22:35:08.572Z
Learning: In the munich-quantum-toolkit/core repository's nanobind bindings, use `.sig("...")` on parameter arguments that have vector or container defaults (e.g., `"sites"_a.sig("...") = std::vector<fomac::Session::Device::Site>{}`) to prevent exposing mutable defaults in the Python API, which would be flagged as a code smell by Python linters. This pattern is preferred over removing `.sig("...")` even though it shows `...` in the stub signature.
Applied to files:
src/mqt/predictor/ml/predictor.py
🧬 Code graph analysis (3)
src/mqt/predictor/ml/helper.py (1)
src/mqt/predictor/utils.py (1)
calc_supermarq_features(94-146)
tests/device_selection/test_predictor_ml.py (3)
src/mqt/predictor/ml/helper.py (1)
get_path_training_data(36-38)src/mqt/predictor/rl/helper.py (1)
get_path_training_data(97-99)src/mqt/predictor/ml/predictor.py (3)
predict_device_for_figure_of_merit(847-926)setup_device_predictor(103-167)Predictor(170-844)
src/mqt/predictor/ml/predictor.py (3)
src/mqt/predictor/ml/gnn.py (1)
GNN(191-295)src/mqt/predictor/hellinger/utils.py (1)
get_hellinger_model_path(135-144)src/mqt/predictor/ml/helper.py (11)
TrainingData(616-626)create_dag(192-309)create_feature_vector(164-189)evaluate_classification_model(351-430)evaluate_regression_model(433-494)get_gnn_input_features(312-324)get_path_trained_model(48-50)get_path_trained_model_gnn(53-57)get_path_training_circuits(60-62)get_path_training_circuits_compiled(65-67)get_path_training_data(36-38)
🪛 Ruff (0.14.10)
tests/device_selection/test_predictor_ml.py
42-42: Boolean-typed positional argument in function definition
(FBT001)
42-42: Boolean-typed positional argument in function definition
(FBT001)
90-90: Boolean-typed positional argument in function definition
(FBT001)
90-90: Boolean-typed positional argument in function definition
(FBT001)
170-170: Boolean-typed positional argument in function definition
(FBT001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (15)
src/mqt/predictor/ml/predictor.py (5)
84-88: LGTM!The type aliases are well-defined and properly distinguish between GNN samples (with graph data) and feature samples (with classical feature vectors). The union type
TrainingSamplecleanly supports both paths.
94-101: LGTM!The
TrainGNNKwargsTypedDict withtotal=Falsecorrectly defines optional GNN training parameters that can be passed through**gnn_kwargs.
347-372: LGTM! Safetensors serialization is a good choice.The GNN data serialization using safetensors is secure and efficient. The separate
.labelfile handling for string labels is a reasonable approach since safetensors only supports tensor data.
517-524: LGTM! k-folds guard correctly handles edge cases.The logic properly ensures
2 <= k_folds <= len(dataset)and raises a clear error message for insufficient samples.
883-919: LGTM! GNN inference path is well-structured.The GNN prediction path correctly:
- Loads model metadata from JSON
- Reconstructs the GNN architecture with saved hyperparameters
- Uses
weights_only=Trueandmap_locationfor secure model loading- Properly squeezes the batch dimension from output
tests/device_selection/test_predictor_ml.py (3)
38-43: Boolean parameters in parameterized tests are acceptable.The Ruff FBT001 warnings about boolean-typed positional arguments can be safely ignored here. In pytest parameterized tests, boolean parameters are idiomatic and the
idsparameter makes the test output clear.
148-156: LGTM! Cleanup logic properly handles new GNN artifacts.The cleanup correctly removes:
.npy,.pt,.safetensors,.labelfilesgraph_dataset_*directories with their contents
68-78: LGTM! Artifact assertions properly distinguish GNN and RF paths.The test correctly validates different expected artifacts:
- GNN:
graph_dataset_expected_fidelity/directory with.safetensorsfiles- RF:
training_data_expected_fidelity.npyBoth paths verify
names_listandscores_listexist.src/mqt/predictor/ml/helper.py (7)
119-152: LGTM! OpenQASM 3.0 gate list with documentation link.The gate list includes a reference to the OpenQASM 3.0 specification and notes about verification, which aids future maintenance.
192-216: LGTM! DAG creation with proper error handling.The function correctly:
- Removes barriers before processing
- Wraps transpilation in try-except with descriptive error message
- Uses
_safe_floatto handle symbolic parameters gracefully
278-309: LGTM! Critical path detection is robust.The implementation:
- Uses defensive
.get(node, 0)access consistently- Handles empty
topo_nodesedge case- Correctly computes longest path using forward/backward passes
327-345: LGTM!get_results_classescorrectly handles both encoding types.The function properly distinguishes:
- 1D integer labels: uses directly as
.long()- [N, 1] shaped: views as 1D and converts to
.long()- One-hot encoded: uses
argmax(dim=1)This addresses the past review concern about corrupted target labels.
405-407: LGTM! Empty loader guard preventstorch.catcrash.The early return when
all_predsorall_targetsis empty correctly prevents the runtime error that would occur when concatenating empty lists.
588-600: LGTM! Verbose output correctly distinguishes task types.Classification displays accuracy; regression displays R² — no longer mixing irrelevant metrics.
615-626: LGTM! TrainingData dataclass supports both classical and GNN data.The widened type annotations correctly support:
- Classical:
NDArray[np.float64]for features- GNN:
list[torch_geometric.data.Data]for graph dataOptional fields with
Nonedefaults handle cases where test data isn't available.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/device_selection/test_helper_ml.py:
- Around line 41-58: The test misses asserting the feature dimension for node
vectors; update test_empty_circuit_dag to also verify node_vector.shape[1]
equals the expected feature size used by create_dag (referencing the constant or
value used by create_dag, e.g., FEATURE_DIM or the hardcoded feature length in
the DAG feature construction). Locate create_dag and confirm the exact feature
dimension symbol or value, then add an assertion using that symbol (or the
literal expected integer) to ensure the second dimension of node_vector matches
the DAG feature dimension.
- Around line 34-38: The test test_create_dag incorrectly treats the return of
create_dag as a single value and uses a weak assertion; unpack the returned
tuple into node_vector, edge_index, number_nodes (from create_dag(qc)) and
replace the trivial assert with meaningful checks: ensure node_vector is not
None and has expected shape/length, edge_index is a sequence/array of edges
(non-empty if expected), and number_nodes equals the length of node_vector (or a
computed expected count from get_benchmark(...).decompose()). Use the existing
get_benchmark and BenchmarkLevel.INDEP/decompose call to derive any expected
size for stronger assertions.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/device_selection/test_helper_ml.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/device_selection/test_helper_ml.py
🧬 Code graph analysis (1)
tests/device_selection/test_helper_ml.py (1)
src/mqt/predictor/ml/helper.py (1)
create_dag(192-309)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
🔇 Additional comments (1)
tests/device_selection/test_helper_ml.py (1)
13-13: LGTM!The new imports are necessary for the DAG tests and are used appropriately.
Also applies to: 15-15, 18-18
| def test_empty_circuit_dag() -> None: | ||
| """Test the creation of a DAG from an empty quantum circuit.""" | ||
| qc = QuantumCircuit(2) | ||
|
|
||
| node_vector, edge_index, number_nodes = create_dag(qc) | ||
|
|
||
| # No nodes | ||
| assert number_nodes == 0 | ||
|
|
||
| # node_vector empty with shape | ||
| assert isinstance(node_vector, torch.Tensor) | ||
| assert node_vector.ndim == 2 | ||
| assert node_vector.shape[0] == 0 | ||
|
|
||
| # edge_index empty (2, 0) and dtype long as in the code | ||
| assert isinstance(edge_index, torch.Tensor) | ||
| assert edge_index.dtype == torch.long | ||
| assert tuple(edge_index.shape) == (2, 0) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Consider verifying the feature dimension.
The test correctly validates the empty circuit edge case with proper tuple unpacking and comprehensive assertions for shapes and types.
📊 Optional: Add feature dimension verification
For completeness, you could also verify that the feature dimension is correct:
# node_vector empty with shape
assert isinstance(node_vector, torch.Tensor)
assert node_vector.ndim == 2
assert node_vector.shape[0] == 0
+ # Feature dimension should be: len(gates) + 6(params) + 1(arity) + 1(controls) + 1(num_params) + 1(critical) + 1(fan_in) + 1(fan_out)
+ # This ensures the tensor structure is correct even when empty
+ assert node_vector.shape[1] > 0🤖 Prompt for AI Agents
In @tests/device_selection/test_helper_ml.py around lines 41 - 58, The test
misses asserting the feature dimension for node vectors; update
test_empty_circuit_dag to also verify node_vector.shape[1] equals the expected
feature size used by create_dag (referencing the constant or value used by
create_dag, e.g., FEATURE_DIM or the hardcoded feature length in the DAG feature
construction). Locate create_dag and confirm the exact feature dimension symbol
or value, then add an assertion using that symbol (or the literal expected
integer) to ensure the second dimension of node_vector matches the DAG feature
dimension.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/predictor/ml/predictor.py (1)
374-450: Align_generate_training_sampletype hints with actual usage
_generate_training_sampleis annotated withfile: Path, butgenerate_training_datapassesfilename.name(astr). Inside the method you treatfileas bothstrandPath(str(file),path_uncompiled_circuit / file), so the runtime code works but the signature is misleading for type checkers.Consider relaxing the annotation to reflect actual usage:
Suggested diff
- def _generate_training_sample( - self, - file: Path, + def _generate_training_sample( + self, + file: Path | str,This avoids confusing static analysis without changing behavior.
🤖 Fix all issues with AI agents
In @src/mqt/predictor/ml/predictor.py:
- Around line 332-343: The GNN training Data is created with an extra leading
dimension because y is built using .unsqueeze(0) which produces shape [1,
num_devices] per sample and later batches to [batch_size, 1, num_devices],
causing a mismatch with the model output; fix this by constructing the target
tensor for gnn_training_sample without the unsqueeze so y is a 1-D tensor of
device scores (use torch.tensor(value_device, dtype=torch.float32)) when
building Data in the block that creates gnn_training_sample (the Data(...) call
referenced by variable gnn_training_sample and guarded by self.gnn) so batched
targets match the GNN output shape expected by evaluate_classification_model.
In @tests/device_selection/test_predictor_ml.py:
- Around line 86-128: The test function
test_setup_multidevice_predictor_with_prediction_gnn uses an undefined variable
gnn in the if/else block; since the test calls setup_device_predictor(...,
gnn=True, ...) and predict_device_for_figure_of_merit(..., gnn=True), remove the
conditional and always assert the GNN-specific artifacts (check
graph_dataset_expected_fidelity exists and contains .safetensors and the
names/scores numpy files) and keep the final prediction call asserting
predicted.description; update the assertions in the function body accordingly to
reference the GNN artifact paths.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.pytests/device_selection/test_predictor_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/device_selection/test_predictor_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:25.619Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:25.619Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with an extensive set of rules including SLF (flake8-self) in the extend-select list. The `# noqa: SLF001` directive for private member access (e.g., `self.state._layout = self.layout` in src/mqt/predictor/rl/predictorenv.py) is necessary and appropriate, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:10.402Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:10.402Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with the "SLF" (flake8-self) rule category enabled in extend-select. SLF001 (private member access) is active, so `# noqa: SLF001` directives are necessary when accessing private methods/attributes (e.g., `_ensure_device_averages_cached()`), even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-25T13:28:19.850Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:19.850Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff has a broad rule set including FLake8-SLF (slf) in extend-select. For private member access (e.g., self.state._layout = self.layout) in src/mqt/predictor/rl/predictorenv.py, include a per-file noqa directive: # noqa: SLF001. This is appropriate even if Ruff reports RUF100 as unused, to acknowledge intentional private attribute access and to avoid false positives in this specific code path. Apply this directive only to the files where private-member access is intentionally used and where SLF001 is the correct rationale.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
📚 Learning: 2025-12-21T22:35:08.572Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/fomac/fomac.cpp:348-364
Timestamp: 2025-12-21T22:35:08.572Z
Learning: In the munich-quantum-toolkit/core repository's nanobind bindings, use `.sig("...")` on parameter arguments that have vector or container defaults (e.g., `"sites"_a.sig("...") = std::vector<fomac::Session::Device::Site>{}`) to prevent exposing mutable defaults in the Python API, which would be flagged as a code smell by Python linters. This pattern is preferred over removing `.sig("...")` even though it shows `...` in the stub signature.
Applied to files:
src/mqt/predictor/ml/predictor.py
🧬 Code graph analysis (4)
tests/device_selection/test_predictor_ml.py (1)
src/mqt/predictor/ml/predictor.py (1)
predict_device_for_figure_of_merit(842-921)
tests/hellinger_distance/test_estimated_hellinger_distance.py (5)
src/mqt/predictor/ml/helper.py (3)
TrainingData(614-624)create_dag(192-309)get_path_training_data(36-38)src/mqt/predictor/rl/helper.py (1)
get_path_training_data(97-99)src/mqt/predictor/ml/predictor.py (1)
Predictor(170-839)tests/compilation/test_reward.py (1)
device(38-40)src/mqt/predictor/hellinger/utils.py (2)
calc_device_specific_features(37-132)hellinger_distance(29-34)
src/mqt/predictor/ml/helper.py (1)
src/mqt/predictor/utils.py (1)
calc_supermarq_features(94-146)
src/mqt/predictor/ml/predictor.py (3)
src/mqt/predictor/ml/gnn.py (1)
GNN(191-295)src/mqt/predictor/hellinger/utils.py (1)
get_hellinger_model_path(135-144)src/mqt/predictor/ml/helper.py (8)
TrainingData(614-624)create_dag(192-309)create_feature_vector(164-189)evaluate_classification_model(351-430)evaluate_regression_model(433-494)get_gnn_input_features(312-324)get_path_trained_model(48-50)get_path_trained_model_gnn(53-57)
🪛 Ruff (0.14.10)
tests/device_selection/test_predictor_ml.py
42-42: Boolean-typed positional argument in function definition
(FBT001)
42-42: Boolean-typed positional argument in function definition
(FBT001)
113-113: Undefined name gnn
(F821)
167-167: Boolean-typed positional argument in function definition
(FBT001)
tests/hellinger_distance/test_estimated_hellinger_distance.py
171-171: Boolean-typed positional argument in function definition
(FBT001)
240-240: Boolean-typed positional argument in function definition
(FBT001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (2)
tests/hellinger_distance/test_estimated_hellinger_distance.py (1)
37-47: GNN optional-dependency handling and cleanup fixture look solidThe
_HAS_GNN_DEPSflag plus earlypytest.skipin the GNN parametrizations, together with thecleanup_artifactsautouse fixture, make the Hellinger tests robust both with and without torch/torch-geometric installed and avoid test-order dependencies. No issues from a functional or maintainability perspective.Also applies to: 168-176, 236-247, 346-393
src/mqt/predictor/ml/helper.py (1)
192-309: DAG featurization and GNN input-dimension helper are internally consistent
create_dag’s feature layout (one‑hot overget_openqasm3_gates()+"measure", 6 param features, arity, controls, num_params, critical flag, fan_in, fan_out) matchesget_gnn_input_features(), and the transpilation/critical-path logic handles empty/degenerate circuits defensively. This provides a solid, well-defined input space for the GNN models.Also applies to: 312-325
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/device_selection/test_predictor_ml.py (1)
126-149: Cleanup test handles new GNN artifacts but still relies on test orderingThe extended cleanup in
test_remove_files(including.safetensors/.labelandgraph_dataset_*directories) matches the new artifact layout, but as a standalone test it still depends on execution order and-kselection. Consider migrating this logic into an autouse fixture (as done in the Hellinger tests) so cleanup runs regardless of which tests are selected or in what order.tests/hellinger_distance/test_estimated_hellinger_distance.py (1)
333-343: Minor type alignment forTrainingDataconstruction in negative testIn
test_predict_device_for_estimated_hellinger_distance_no_device_provided,TrainingData.y_trainis passed as a plain Python list. This works with scikit‑learn, but is slightly out of sync with the annotated type (NDArray[np.float64] | torch.Tensor). For consistency with other tests and stricter type-checking, consider wrappinglabels_listinnp.asarray(..., dtype=np.float64)here as well.src/mqt/predictor/ml/predictor.py (3)
842-915: Address PyTorch version dependency and missing error handling forweights_only=TrueThe state_dict + model reconstruction approach is sound, but the code has critical gaps:
PyTorch version constraint:
weights_only=Trueis not a PyTorch 2.1 feature; its behavior is only documented consistently in PyTorch >=2.6. If the project targets earlier versions, this parameter will fail silently or error at load time.No error handling for checkpoint incompatibility:
weights_only=Trueraises "Weights only load failed" if the checkpoint contains non-tensor objects or custom tensor subclasses. The code has no try/except aroundtorch.load, so it will crash if the saved GNN checkpoint is incompatible.Missing post-load validation: After loading the state dict, validate that tensor shapes and dtypes match expectations before inference.
Add a PyTorch version check (require >=2.6 for
weights_only=True), add error handling aroundtorch.loadwith fallback toweights_only=Falseand logging, and validate tensor properties after load. Alternatively, considersafetensorsfor more robust, format-agnostic loading.
709-751: The CV clamping logic risks failure with min_class < 2; adjust to handle extreme class imbalanceThe CV guard:
num_cv = min(len(training_data.y_train), 5) min_class = min(Counter(training_data.y_train).values()) num_cv = max(2, min(num_cv, min_class))enforces a minimum of
cv=2, which causesGridSearchCVto fail when the smallest class has only 1 sample. SinceGridSearchCVusesStratifiedKFoldby default for classifiers and requiresn_splits ≤ min_class_count, the current logic will raise aValueErrorin these edge cases.Consider either:
- Removing the
max(2, ...)guard and raising a clearValueErrorwhenmin_class < 2, or- Skipping stratified CV (e.g.,
cv=1) whenmin_class < 2and documenting this limitation.This ensures robustness for datasets with rare classes while maintaining stratified validation when sufficient samples exist.
318-346: Critical: extraunsqueeze(0)on GNN targets breaks shape compatibility during batchingIn
generate_training_data, the GNN branch stores targets as:gnn_training_sample = Data( x=x, y=torch.tensor(value_device, dtype=torch.float32).unsqueeze(0), edge_index=edge_idx, num_nodes=n_nodes, target_label=target_label, )This creates each
Data.ywith shape(1, num_devices). When batched viatorch_geometric.loader.DataLoader,batch.ybecomes(batch_size, 1, num_devices), whereas the GNN model outputs shape(batch_size, num_devices). Sinceevaluate_classification_modelenforces shape equality (line 391 in helper.py), MSELoss and any loss computation will fail withValueError: Shape mismatch ...during Optuna trials and final training.Remove the
unsqueeze(0)to store targets as 1D per-device score vectors, and add backward-compatibility handling for legacy(1, N)tensors when loading safetensors:Proposed fix
if self.gnn: x, _y, edge_idx, n_nodes, target_label = training_sample value_device = [scores.get(dev.description, -1.0) for dev in self.devices] - gnn_training_sample = Data( - x=x, - y=torch.tensor(value_device, dtype=torch.float32).unsqueeze(0), - edge_index=edge_idx, - num_nodes=n_nodes, - target_label=target_label, - ) + # Store targets as 1D per-device score vectors so batched shapes + # match model outputs: (batch_size, num_devices). + gnn_training_sample = Data( + x=x, + y=torch.tensor(value_device, dtype=torch.float32), + edge_index=edge_idx, + num_nodes=n_nodes, + target_label=target_label, + )In
_get_prepared_training_data, add backward-compatibility squeeze:tensors = load_file(str(sf)) + y = tensors["y"] + # Backward compatibility: squeeze legacy (1, N) targets to (N,) + if y.dim() == 2 and y.size(0) == 1: + y = y.squeeze(0) data = Data( x=tensors["x"], - y=tensors["y"], + y=y, edge_index=tensors["edge_index"], num_nodes=int(tensors["num_nodes"][0].item()), )
🤖 Fix all issues with AI agents
In @tests/hellinger_distance/test_estimated_hellinger_distance.py:
- Around line 26-47: Replace the try/except block that sets torch, Batch, Data
and _HAS_GNN_DEPS with pytest.importorskip calls: call
pytest.importorskip("torch", exc_type=ModuleNotFoundError) and
pytest.importorskip("torch_geometric", exc_type=ModuleNotFoundError) (or the
specific subpackage names you need) at the top of the test module, then import
torch and torch_geometric symbols (Batch, Data) normally and remove the
_HAS_GNN_DEPS flag and its usages; this surfaces real import failures while
making the tests skip cleanly when the optional GNN deps are missing.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.pytests/device_selection/test_predictor_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/device_selection/test_predictor_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:25.619Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:25.619Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with an extensive set of rules including SLF (flake8-self) in the extend-select list. The `# noqa: SLF001` directive for private member access (e.g., `self.state._layout = self.layout` in src/mqt/predictor/rl/predictorenv.py) is necessary and appropriate, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:10.402Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:10.402Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with the "SLF" (flake8-self) rule category enabled in extend-select. SLF001 (private member access) is active, so `# noqa: SLF001` directives are necessary when accessing private methods/attributes (e.g., `_ensure_device_averages_cached()`), even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-25T13:28:19.850Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:19.850Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff has a broad rule set including FLake8-SLF (slf) in extend-select. For private member access (e.g., self.state._layout = self.layout) in src/mqt/predictor/rl/predictorenv.py, include a per-file noqa directive: # noqa: SLF001. This is appropriate even if Ruff reports RUF100 as unused, to acknowledge intentional private attribute access and to avoid false positives in this specific code path. Apply this directive only to the files where private-member access is intentionally used and where SLF001 is the correct rationale.
Applied to files:
src/mqt/predictor/ml/helper.pysrc/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/mqt/predictor/ml/helper.py
📚 Learning: 2025-12-21T22:35:08.572Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/fomac/fomac.cpp:348-364
Timestamp: 2025-12-21T22:35:08.572Z
Learning: In the munich-quantum-toolkit/core repository's nanobind bindings, use `.sig("...")` on parameter arguments that have vector or container defaults (e.g., `"sites"_a.sig("...") = std::vector<fomac::Session::Device::Site>{}`) to prevent exposing mutable defaults in the Python API, which would be flagged as a code smell by Python linters. This pattern is preferred over removing `.sig("...")` even though it shows `...` in the stub signature.
Applied to files:
src/mqt/predictor/ml/predictor.py
🧬 Code graph analysis (3)
tests/device_selection/test_predictor_ml.py (2)
src/mqt/predictor/ml/helper.py (1)
get_path_training_data(36-38)src/mqt/predictor/ml/predictor.py (3)
predict_device_for_figure_of_merit(842-921)setup_device_predictor(103-167)Predictor(170-839)
tests/hellinger_distance/test_estimated_hellinger_distance.py (5)
src/mqt/predictor/ml/helper.py (3)
TrainingData(614-624)create_dag(192-309)get_path_training_data(36-38)src/mqt/predictor/rl/helper.py (1)
get_path_training_data(97-99)src/mqt/predictor/ml/predictor.py (4)
Predictor(170-839)train_gnn_model(581-707)train_random_forest_model(709-756)compile_training_circuits(252-283)src/mqt/predictor/rl/predictor.py (1)
Predictor(32-127)src/mqt/predictor/hellinger/utils.py (1)
hellinger_distance(29-34)
src/mqt/predictor/ml/helper.py (2)
src/mqt/predictor/utils.py (1)
calc_supermarq_features(94-146)src/mqt/predictor/rl/helper.py (1)
get_path_training_data(97-99)
🪛 Ruff (0.14.10)
tests/device_selection/test_predictor_ml.py
42-42: Boolean-typed positional argument in function definition
(FBT001)
42-42: Boolean-typed positional argument in function definition
(FBT001)
162-162: Boolean-typed positional argument in function definition
(FBT001)
tests/hellinger_distance/test_estimated_hellinger_distance.py
171-171: Boolean-typed positional argument in function definition
(FBT001)
240-240: Boolean-typed positional argument in function definition
(FBT001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (13)
src/mqt/predictor/ml/helper.py (3)
53-57: GNN path/file helpers look consistent and well-scoped
get_path_trained_model_gnnandget_openqasm3_gatesare straightforward and match how the GNN artifacts and OpenQASM 3 basis are used elsewhere. Path construction is stable (reusingget_path_training_data) and the gate list is clearly tied to the spec URL and comment, which should make future maintenance easier.Also applies to: 119-152
192-310: DAG featurization and input-dimension helper are internally consistentThe
create_dagpipeline (barrier removal, controlled transpile with a clear error, op-node enumeration, feature tensors, edge_index, and critical-path tagging) is coherent. The finalnode_vectordimension matchesget_gnn_input_features()(one‑hot overget_openqasm3_gates() + ["measure"], 6 sin/cos params, and 6 scalar DAG attributes), including empty‑DAG and no‑topo‑nodes edge cases. The use of.get(..., 0)fordist_in/dist_outand an explicit empty-edge tensor avoids KeyErrors and shape issues.Also applies to: 312-324
327-346: Evaluation helpers andTrainingDatatyping align with mixed RF/GNN use
get_results_classes,evaluate_classification_model,evaluate_regression_model, andtrain_modelhandle both regression and classification cleanly, including:
- robust device handling (string vs
torch.device),- empty-loader guards before
torch.cat,- consistent metrics dict typing (
dict[str, float | str]),- and early‑stopping logic that always initializes
val_loss/val_metricsbefore use.The updated
TrainingDataannotation matches how RF (numpy arrays) and GNN (list oftorch_geometric.data.Data) flows actually consumeX_*/y_*. This should also satisfy the earlier static-analysis concerns about potentially uninitialized locals.Also applies to: 351-430, 433-495, 497-611, 613-620
tests/device_selection/test_predictor_ml.py (2)
38-64: End‑to‑end RF/GNN setup test correctly exercises both backendsThe parametrized
test_setup_device_predictor_with_predictionwiresgnnandverbosethrough tosetup_device_predictorandpredict_device_for_figure_of_meritas intended, and the artifact assertions match the RF vs GNN storage layout (training_data_expected_fidelity.npyvsgraph_dataset_expected_fidelitywith.safetensorsplusnames_list/scores_list). The final device prediction check against"ibm_falcon_127"gives a clear functional smoke test for both backends.Also applies to: 67-84
86-124: Multi‑device GNN test validates graph dataset layout and prediction
test_setup_multidevice_predictor_with_prediction_gnncorrectly:
- trains with
gnn=Trueon two devices,- asserts that the
graph_dataset_expected_fidelitydirectory exists and contains at least one.safetensorsfile plus the sharednames_list/scores_list,- and verifies that the predicted device is one of the two configured backends.
This gives good coverage of the multi‑device GNN path.
tests/hellinger_distance/test_estimated_hellinger_distance.py (3)
168-234: RF/GNN Hellinger training test is consistent; tolerance is intentionally loose
test_train_model_and_predictcorrectly:
- uses
model_typeto avoid boolean parameter lint and to derivegnn,- skips the GNN branch when
_HAS_GNN_DEPSis false,- builds RF training data from device‑specific feature vectors,
- builds GNN training data from
create_daggraphs with scalarytargets (suitable for regression),- and exercises both
train_random_forest_modelandtrain_gnn_model.The relatively loose
atol=5e-1check for the GNN branch is well‑documented in the comments as a smoke‑test tolerance given very small training sets, which is acceptable here.
236-323: End‑to‑end estimated‑Hellinger toolchain test covers RF and GNN paths
test_train_and_qcompile_with_hellinger_modelnow:
- parametrizes over RF vs GNN backends and verbosity,
- skips GNN variants when GNN deps are missing,
- trains a small RL model for compilation, then uses
ml_Predictorwithgnnwired through to compile circuits, generate training data, and train the RF/GNN models,- asserts graph datasets (
graph_dataset_estimated_hellinger_distance) and safetensors layout for GNN, or the legacy.npyfiles for RF,- and validates that
predict_device_for_figure_of_meritreturns a device fromget_available_device_names().This gives good coverage of the full RL+ML pipeline across both backends.
346-393: Autouse cleanup fixture matches new artifact layout and is reasonably conservativeThe
cleanup_artifactsfixture:
- runs once per module (autouse) and performs teardown after all tests,
- removes only
.qasmfiles in the per-test source/target directories and then attempts to rmdir them,- prunes training data artifacts matching known extensions plus
graph_dataset_*directories (with a small safeguard for nested dirs),- and cleans up trained model files in
trained_model.This keeps test artifacts under control without resorting to unbounded
rmtree, and the NOTE about potential flakiness is appropriate.src/mqt/predictor/ml/predictor.py (5)
18-24: Type aliases and GNN training kwargs are well-structured
GNNSample/FeatureSample/TrainingSampleclearly describe the two training-sample shapes, andTrainGNNKwargsgives a typed surface for GNN‑only options. Makinggnnkeyword‑only in bothsetup_device_predictorandPredictor.__init__, and forwarding**gnn_kwargsintotrain_gnn_model, keeps the public API explicit without cluttering the core signature.Also applies to: 25-26, 77-88, 94-101
374-449: GNN training sample generation and DAG path integration are structurally sound aside from target shapeApart from the
unsqueeze(0)issue ony, the GNN branch of_generate_training_sampleandgenerate_training_datacorrectly:
- compute scores per device via the configured figure of merit,
- skip samples where all devices are
-1.0(no compiled circuits),- derive a canonical device ordering from
self.devices,- build
create_dag(qc)features and wrap them intorch_geometric.data.Dataobjects annotated withtarget_label,- and serialize graph datasets plus
.labelfiles undergraph_dataset_{figure_of_merit}.Once the target shape is fixed as per the other comment, the end-to-end GNN dataset pipeline should be consistent with
create_dagandget_gnn_input_features().
581-707:train_gnn_modelend-to-end flow is coherent (data prep, Optuna, final training, metadata, checkpointing)
train_gnn_modelcorrectly:
- chooses
num_outputsand save path based on the figure of merit (single-output regression for Hellinger vs multi-output for device scoring),- falls back to
_get_prepared_training_data()whentraining_datais not provided,- uses
get_gnn_input_features()andnn.MSELoss()consistently withcreate_dag,- runs an Optuna study via the
objectivehelper,- rebuilds the best GNN with parsed
mlp_unitsandnum_outputs,- writes a JSON config containing hyperparameters and
class_labels,- performs a second training run with early stopping on a held-out validation split,
- optionally evaluates on a test split when present, and
- saves only the model
state_dict(good for later secure loading).Once the GNN target shape in the dataset is corrected, this orchestration should train and persist models suitable for the downstream prediction path.
758-839: Prepared-data loader cleanly supports both RF and GNN, including legacy scores format
_get_prepared_training_datanow:
- branches correctly between RF (
training_data_{figure}.npy) and GNN (graph_dataset_{figure}safetensors),- reconstructs
Dataobjects with integernum_nodesand reattachestarget_labelstrings,- normalizes
scores_listso both legacy dict entries and new list/array entries becomelist[list[float]],- and returns a
TrainingDatainstance whose fields match the updated dataclass types for both dense and graph training.Once the safetensors loader squeezes any legacy
(1, N)targets (per the earlier suggestion), this loader provides a solid bridge between stored artifacts and the new GNN training code.
758-839: RF/GNNTrainingDataunpacking matches dataclass expectationsThe RF branch unpacks
training_datainto(feature_vec, target_label)pairs and converts them into dense numpy arrays, while the GNN branch returnsxas a list ofDataandyas an array oftarget_labelstrings. The subsequent 70/30 split and reconstruction ofTrainingData(includingnames_listand normalizedscores_list) is consistent with both the RF and GNN training code paths.
| def objective( | ||
| self, | ||
| trial: optuna.Trial, | ||
| dataset: NDArray[np.float64] | list[torch_geometric.data.Data], | ||
| task: str, | ||
| in_feats: int, | ||
| num_outputs: int, | ||
| loss_fn: nn.Module, | ||
| k_folds: int, | ||
| batch_size: int = 32, | ||
| num_epochs: int = 10, | ||
| patience: int = 10, | ||
| device: str | None = None, | ||
| *, | ||
| verbose: bool = False, | ||
| ) -> float: | ||
| """Objective function for Optuna GNN hyperparameter optimization. | ||
| Arguments: | ||
| trial: The Optuna trial object. | ||
| dataset: Dataset used for training and validation (classical or GNN). | ||
| task: Task type, e.g. "classification", or "regression". | ||
| in_feats: Number of input node features. | ||
| num_outputs: Number of model outputs (classes or regression targets). | ||
| loss_fn: Loss function to optimize. | ||
| k_folds: Number of cross-validation folds. | ||
| batch_size: Batch size for training. | ||
| num_epochs: Maximum number of training epochs per fold. | ||
| patience: Early-stopping patience (epochs without improvement). | ||
| device: Device to use for training (e.g. "cpu" or "cuda"), or None. | ||
| verbose: Whether to print verbose output during training. | ||
| Returns: | ||
| mean_val: The mean validation metric considering the k-folds. | ||
| """ | ||
| # Type of device used | ||
| if device is None: | ||
| device = "cuda" if torch.cuda.is_available() else "cpu" | ||
| device_obj = torch.device(device) | ||
|
|
||
| hidden_dim = trial.suggest_int("hidden_dim", 8, 64) | ||
| num_conv_wo_resnet = trial.suggest_int("num_conv_wo_resnet", 1, 3) | ||
| num_resnet_layers = trial.suggest_int("num_resnet_layers", 1, 9) | ||
| dropout = trial.suggest_categorical("dropout", [0.0, 0.1, 0.2, 0.3]) | ||
| sag_pool = trial.suggest_categorical("sag_pool", [False, True]) | ||
| bidirectional = trial.suggest_categorical("bidirectional", [False, True]) # True, False]) | ||
| mlp_options = [ | ||
| "none", | ||
| "32", | ||
| "64", | ||
| "128", | ||
| "256", | ||
| "64,32", | ||
| "128,32", | ||
| "128,64", | ||
| "256,32", | ||
| "256,64", | ||
| "256,128", | ||
| "128,64,32", | ||
| "256,128,64", | ||
| ] | ||
|
|
||
| mlp_str = trial.suggest_categorical("mlp", mlp_options) | ||
| mlp_units = [] if mlp_str == "none" else [int(x) for x in mlp_str.split(",")] | ||
| lr = trial.suggest_categorical("lr", [1e-2, 1e-3, 1e-4]) | ||
| # Ensure at least 2 folds | ||
| max_splits = len(dataset) | ||
| k_folds = min(max(2, k_folds), max_splits) | ||
| if k_folds < 2: | ||
| msg = f"Not enough samples ({len(dataset)}) for k-folds (k={k_folds})." | ||
| raise ValueError(msg) | ||
| # Split into k-folds | ||
| kf = KFold(n_splits=k_folds, shuffle=True) | ||
| fold_val_best_losses: list[float] = [] | ||
| for _fold_idx, (train_idx, val_idx) in enumerate(kf.split(range(len(dataset)))): | ||
| train_subset = [dataset[i] for i in train_idx] | ||
| val_subset = [dataset[i] for i in val_idx] | ||
| # Transform the data into loaders | ||
| train_loader = DataLoader(train_subset, batch_size=batch_size, shuffle=True) | ||
| val_loader = DataLoader(val_subset, batch_size=batch_size, shuffle=False) | ||
| # Define the GNN | ||
| model = GNN( | ||
| in_feats=in_feats, | ||
| num_conv_wo_resnet=num_conv_wo_resnet, | ||
| hidden_dim=hidden_dim, | ||
| num_resnet_layers=num_resnet_layers, | ||
| mlp_units=mlp_units, | ||
| output_dim=num_outputs, | ||
| dropout_p=dropout, | ||
| bidirectional=bidirectional, | ||
| use_sag_pool=sag_pool, | ||
| sag_ratio=0.7, | ||
| conv_activation=torch.nn.functional.leaky_relu, | ||
| mlp_activation=torch.nn.functional.leaky_relu, | ||
| ).to(device_obj) | ||
|
|
||
| optimizer = torch.optim.Adam(model.parameters(), lr=lr) | ||
| # Based on the task, do a training and evaluation for regression or classification | ||
| train_model( | ||
| model, | ||
| train_loader, | ||
| optimizer, | ||
| loss_fn, | ||
| task=task, | ||
| num_epochs=num_epochs, | ||
| device=device, | ||
| verbose=verbose, | ||
| val_loader=val_loader, | ||
| patience=patience, | ||
| min_delta=0.0, | ||
| restore_best=True, | ||
| ) | ||
| if task == "regression": | ||
| val_loss, _, _ = evaluate_regression_model( | ||
| model, val_loader, loss_fn, device=device, return_arrays=False, verbose=verbose | ||
| ) | ||
| else: | ||
| val_loss, _, _ = evaluate_classification_model( | ||
| model, val_loader, loss_fn, device=device, return_arrays=False, verbose=verbose | ||
| ) | ||
|
|
||
| fold_val_best_losses.append(float(val_loss)) | ||
| del train_loader, val_loader, train_subset, val_subset, optimizer, model | ||
| if device_obj.type == "cuda": | ||
| torch.cuda.empty_cache() | ||
| gc.collect() | ||
| # Take the mean value | ||
| return float(np.mean(fold_val_best_losses)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
scikit-learn KFold StratifiedKFold small class count imbalanced dataset single sample per class
💡 Result:
Short answer: StratifiedKFold needs at least n_splits samples in every class. If any class has < n_splits (e.g., a class with only one sample) StratifiedKFold will warn/error and cannot guarantee stratified folds. [1][2]
Options (pick one):
- Lower n_splits so n_splits <= min_class_count. [1]
- Use non‑stratified KFold (shuffle=True) — loses per‑fold class proportion guarantees.
- Use StratifiedShuffleSplit with a properly chosen train/test sizes (it’s more flexible than KFold but has quirks for very tiny classes). [4][3]
- Use LeaveOneOut (or LeavePOut) for tiny datasets so every sample gets tested.
- Resample the data (oversample rare classes, synthetic methods like SMOTE, or upsample the single sample) before CV.
- Create a custom splitter that forces singletons into the training set (or uses grouping) or use StratifiedGroupKFold if groups apply. [5][3]
References:
- StratifiedKFold docs (requirement n_splits ≤ members per class). [1]
- Example/stack‑overflow discussion of the “least populated class has only 1 member” warning. [2]
- StratifiedShuffleSplit docs and related GitHub issues about tiny‑class behaviour. [4][3]
- Feature request / discussion about handling single‑sample classes in StratifiedKFold. [5]
If you want, tell me: your y value counts (value_counts) and desired n_splits, and I’ll recommend the best concrete CV approach and show code.
🏁 Script executed:
cd src/mqt/predictor/ml && head -20 predictor.py | tail -10
grep -n "from sklearn" predictor.py | head -20
grep -n "KFold\|StratifiedKFold" predictor.pyRepository: munich-quantum-toolkit/predictor
Length of output: 580
KFold cross-validation handles typical dataset sizes well; consider per-class guarding for highly imbalanced classification
The Optuna objective function:
- samples a reasonable hyperparameter space (hidden_dim, conv layers, MLP layout, dropout, bidirectionality, SAG pooling, lr),
- clamps
k_foldsto2 <= k_folds <= len(dataset)to prevent invalidKFoldconfigurations, - constructs per-fold
DataLoaders and GNN instances on the chosen device, - delegates training to the shared
train_modelhelper (with patience, min_delta, restore_best), - and evaluates via the appropriate regression/classification evaluator.
For classification tasks with highly imbalanced data (e.g., classes with single or very few samples), regular KFold does not enforce per-class stratification and some folds may not include rare classes. While KFold will not fail, this can lead to uneven class representation across folds. For typical datasets this is acceptable, but if you anticipate small or highly imbalanced graph datasets, consider adding an explicit check on per-class sample counts and either reducing k_folds further, using StratifiedKFold (for classification), or raising a clear error.
| from mqt.predictor.hellinger import calc_device_specific_features, hellinger_distance | ||
| from mqt.predictor.ml import Predictor as ml_Predictor | ||
| from mqt.predictor.ml import predict_device_for_figure_of_merit | ||
| from mqt.predictor.ml.helper import TrainingData, get_path_training_data | ||
| from mqt.predictor.ml.helper import TrainingData, create_dag, get_path_training_data | ||
| from mqt.predictor.rl import Predictor as rl_Predictor | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Iterator | ||
|
|
||
| from qiskit.transpiler import Target | ||
|
|
||
| try: | ||
| import torch # type: ignore[import-not-found] | ||
| from torch_geometric.data import Batch, Data # type: ignore[import-not-found] | ||
|
|
||
| _HAS_GNN_DEPS = True | ||
| except ImportError: # pragma: no cover | ||
| torch = None # type: ignore[assignment] | ||
| Batch = None # type: ignore[assignment] | ||
| Data = None # type: ignore[assignment] | ||
| _HAS_GNN_DEPS = False | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
pytest optional dependencies testing pattern best practices importorskip
💡 Result:
Summary best practices for testing optional dependencies with pytest (concise):
-
Prefer pytest.importorskip at module top to skip whole test modules if a dependency is absent:
- e.g. pkg = pytest.importorskip("pkg", minversion="x.y"). This keeps tests clear and avoids runtime failures. [1]
-
Be explicit about which import error you mean:
- Since pytest 8.2, importorskip can take exc_type; prefer exc_type=ModuleNotFoundError so real import errors (broken installs, compilation failures) surface instead of being silently skipped. Example: pytest.importorskip("pkg", exc_type=ModuleNotFoundError). [2]
-
Structure tests for clarity and CI coverage:
- Put dependency-specific tests in separate modules/files (e.g. tests/test_numpy_integration.py) so importorskip at module top cleanly skips them.
- In CI, run a matrix that installs optional extra sets (extras_require) to exercise tests when deps are present, and another job without them to verify graceful skipping.
-
Test both presence and absence paths:
- For presence: run CI jobs that install the optional dep and test real behavior.
- For absence: rely on tests being skipped (importorskip) and add a small unit test that simulates absence via monkeypatching sys.modules or import hooks to assert your code raises a clear error or falls back—this ensures you don’t silently ignore missing-dep logic.
-
Use skipif/pytest.skip when conditional logic must be evaluated at runtime (not import time), and use pytest.mark.parametrize with pytest.param(..., marks=pytest.mark.skipif(...)) for mixed cases. [1]
-
Consider helper libraries/plugins if you want DRY decorators (e.g., pytest-requires / community helpers) or a controlled “defer import” helper that raises SkipTest when optional APIs are actually used. Evaluate these carefully. [3]
Sources:
- pytest skipping/importorskip docs. [1]
- pytest deprecations note re: importorskip exc_type (8.2). [2]
- Example helper approaches / community packages (e.g., generalimport / pytest-requires). [3]
Use pytest.importorskip for optional GNN dependencies
While the try/except ImportError with _HAS_GNN_DEPS flag pattern works functionally, pytest's documented best practice is to use pytest.importorskip at the module or test level for optional dependencies. This approach is more idiomatic and clearer in intent. Additionally, the current pattern can mask real import errors (e.g., broken installations or compilation failures) by catching all ImportError exceptions. Consider refactoring to use pytest.importorskip("torch", exc_type=ModuleNotFoundError) to be explicit about which errors warrant skipping and which should surface as test failures.
🤖 Prompt for AI Agents
In @tests/hellinger_distance/test_estimated_hellinger_distance.py around lines
26 - 47, Replace the try/except block that sets torch, Batch, Data and
_HAS_GNN_DEPS with pytest.importorskip calls: call pytest.importorskip("torch",
exc_type=ModuleNotFoundError) and pytest.importorskip("torch_geometric",
exc_type=ModuleNotFoundError) (or the specific subpackage names you need) at the
top of the test module, then import torch and torch_geometric symbols (Batch,
Data) normally and remove the _HAS_GNN_DEPS flag and its usages; this surfaces
real import failures while making the tests skip cleanly when the optional GNN
deps are missing.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/predictor/ml/predictor.py (1)
843-921: Wrap single Data object withBatch.from_data_list()for proper batching.At line 907, the single
Dataobject is passed directly to the GNN model without a batch attribute, which will beNone. While this may work incidentally, GraphNorm expects batch indices and the pattern is not documented or tested. UseBatch.from_data_list([feature_vector])to ensure proper graph batching, matching the approach used elsewhere in the codebase.
🤖 Fix all issues with AI agents
In @src/mqt/predictor/ml/predictor.py:
- Around line 880-882: The code incorrectly calls the instance method Path.open
as a class method (Path.open(path.with_suffix(".json"), ...)); change this to
call open on the Path instance: replace that line with
path.with_suffix(".json").open(encoding="utf-8") (or use the builtin
open(path.with_suffix(".json"), encoding="utf-8")) so json.load(f) reads from
the correctly opened file handle; the symbols to update are Path.open and
path.with_suffix(".json") where json_dict is assigned.
In @tests/hellinger_distance/test_estimated_hellinger_distance.py:
- Around line 292-293: The inline comment above the assertion using
dataset_dir.iterdir()/f.suffix/f.stem.isdigit() contains a typo: change
"asdefined" to "as defined" so the comment reads that the name starts with a
number and ends with .safetensors "as defined" in the predictor; update only the
comment text near that assert in test_estimated_hellinger_distance.py.
- Around line 186-225: The variable labels is reassigned inside the GNN
evaluation block which shadows the outer labels; rename the inner variable
(e.g., expected_labels) where you currently do labels = np.asarray(labels_list,
dtype=np.float32) and update subsequent references (the assert that compares
predicted_values to labels -> expected_labels) so the outer labels remain
unchanged and the intent is clearer; ensure you only change occurrences in the
GNN evaluation block (the scope around trained_model.eval(), model_device
retrieval, Batch.from_data_list(...) and the final np.allclose assertion).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/mqt/predictor/ml/predictor.pytests/compilation/test_predictor_rl.pytests/device_selection/test_helper_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
tests/device_selection/test_helper_ml.pytests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:02.860Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:02.860Z
Learning: In the munich-quantum-toolkit/predictor repository, when using Ruff with the SLF (flake8-self) rules enabled, explicitly add a noqa: SLF001 directive when accessing private members (e.g., _ensure_device_averages_cached()) in tests under tests/, even if Ruff would report RUF100 that the directive is unused. This applies to Python test files (and related code) that access private attributes or methods, to ensure maintainability and clarity of the intent.
Applied to files:
tests/device_selection/test_helper_ml.pytests/compilation/test_predictor_rl.pytests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-25T13:28:25.619Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:25.619Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with an extensive set of rules including SLF (flake8-self) in the extend-select list. The `# noqa: SLF001` directive for private member access (e.g., `self.state._layout = self.layout` in src/mqt/predictor/rl/predictorenv.py) is necessary and appropriate, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
tests/hellinger_distance/test_estimated_hellinger_distance.py
📚 Learning: 2025-12-21T22:35:08.572Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/fomac/fomac.cpp:348-364
Timestamp: 2025-12-21T22:35:08.572Z
Learning: In the munich-quantum-toolkit/core repository's nanobind bindings, use `.sig("...")` on parameter arguments that have vector or container defaults (e.g., `"sites"_a.sig("...") = std::vector<fomac::Session::Device::Site>{}`) to prevent exposing mutable defaults in the Python API, which would be flagged as a code smell by Python linters. This pattern is preferred over removing `.sig("...")` even though it shows `...` in the stub signature.
Applied to files:
src/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-25T13:28:10.402Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: tests/compilation/test_predictor_rl.py:225-225
Timestamp: 2025-12-25T13:28:10.402Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff is configured with the "SLF" (flake8-self) rule category enabled in extend-select. SLF001 (private member access) is active, so `# noqa: SLF001` directives are necessary when accessing private methods/attributes (e.g., `_ensure_device_averages_cached()`), even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
src/mqt/predictor/ml/predictor.py
📚 Learning: 2025-12-25T13:28:19.850Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/predictor PR: 526
File: src/mqt/predictor/rl/predictorenv.py:271-271
Timestamp: 2025-12-25T13:28:19.850Z
Learning: In the munich-quantum-toolkit/predictor repository, Ruff has a broad rule set including FLake8-SLF (slf) in extend-select. For private member access (e.g., self.state._layout = self.layout) in src/mqt/predictor/rl/predictorenv.py, include a per-file noqa directive: # noqa: SLF001. This is appropriate even if Ruff reports RUF100 as unused, to acknowledge intentional private attribute access and to avoid false positives in this specific code path. Apply this directive only to the files where private-member access is intentionally used and where SLF001 is the correct rationale.
Applied to files:
src/mqt/predictor/ml/predictor.py
🧬 Code graph analysis (3)
tests/device_selection/test_helper_ml.py (1)
src/mqt/predictor/ml/helper.py (1)
create_dag(192-309)
tests/compilation/test_predictor_rl.py (1)
src/mqt/predictor/rl/predictor.py (1)
rl_compile(151-182)
tests/hellinger_distance/test_estimated_hellinger_distance.py (3)
src/mqt/predictor/hellinger/utils.py (1)
calc_device_specific_features(37-132)src/mqt/predictor/ml/predictor.py (3)
Predictor(170-840)predict_device_for_figure_of_merit(843-922)compile_training_circuits(252-283)src/mqt/predictor/ml/helper.py (3)
TrainingData(614-624)create_dag(192-309)get_path_training_data(36-38)
🪛 Ruff (0.14.10)
tests/hellinger_distance/test_estimated_hellinger_distance.py
165-165: Boolean-typed positional argument in function definition
(FBT001)
232-232: Boolean-typed positional argument in function definition
(FBT001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (15)
tests/device_selection/test_helper_ml.py (3)
13-24: LGTM!The new imports for
torch,QuantumCircuit, andcreate_dagare correctly added to support the new DAG creation tests.
34-44: LGTM!The test properly validates DAG creation with assertions for tensor types, shapes, node count consistency, and edge index properties. Using
BenchmarkLevel.INDEPwith.decompose()ensures the circuit has actual gates for meaningful DAG testing.
47-65: LGTM!Excellent coverage of the empty circuit edge case. The assertions correctly verify that
create_dagreturns properly shaped empty tensors while maintaining the expected dimensions for batch compatibility.tests/compilation/test_predictor_rl.py (2)
65-71: LGTM!The test correctly validates that
rl_compileraises appropriateValueErrorexceptions whenfigure_of_meritordeviceisNonewhilepredictor_singletonis alsoNone. This aligns with the function's documented behavior insrc/mqt/predictor/rl/predictor.py.
74-110: No action needed. The device name "ibm_falcon_127" is valid and correctly used throughout the test suite. It is imported and used successfully viaget_device()frommqt.bench.targetsin multiple test files (test_predictor_ml.py, test_estimated_hellinger_distance.py, and others), with assertions verifying the device name works correctly. The parameterization is appropriate.Likely an incorrect or invalid review comment.
tests/hellinger_distance/test_estimated_hellinger_distance.py (2)
39-40: LGTM!Using
pytest.importorskipfortorchandtorch_geometricis appropriate to skip tests gracefully when optional GNN dependencies are not installed.
336-383: LGTM!The cleanup fixture is well-designed with appropriate safety measures:
- Module-scoped and autouse for automatic cleanup after all tests in the module
- Conservative deletion (only specific file extensions) to avoid accidental data loss
- Helpful comment explaining the rmdir limitation and suggesting shutil.rmtree if needed
- Proper handling of nested directories within graph_dataset_* folders
src/mqt/predictor/ml/predictor.py (8)
18-50: LGTM!The new imports are well-organized and necessary for GNN support. The separation between runtime imports and TYPE_CHECKING imports is correctly maintained.
85-100: LGTM!The type aliases
GNNSample,FeatureSample, andTrainingSampleprovide clear documentation of the data structures used in both RF and GNN paths. TheTrainGNNKwargsTypedDict withtotal=Falsecorrectly allows optional keyword arguments.
103-167: LGTM!The
setup_device_predictorfunction is correctly extended to support GNN training while maintaining backward compatibility with the defaultgnn=False. The conditional training logic and updated logging are appropriate.
327-373: LGTM!The training data generation correctly handles both RF and GNN paths:
- GNN samples are properly constructed as
torch_geometric.data.Dataobjects- The
unsqueeze(0)on line 338 ensures proper batch dimension handling- Using safetensors for tensor serialization and separate
.labelfiles for string labels is a clean approach
453-580: LGTM!The Optuna objective function is well-designed:
- Comprehensive hyperparameter search space covering architecture (layers, hidden dims, MLP) and regularization (dropout, pooling)
- Proper k-fold validation with early stopping
- Good resource management with explicit cleanup (
del,cuda.empty_cache(),gc.collect())- Appropriate validation of minimum fold count
582-708: LGTM!The
train_gnn_modelmethod implements a complete GNN training pipeline:
- Optuna-based hyperparameter optimization with TPE sampler
- JSON configuration saved alongside the model for reproducible inference
- Final model training with early stopping after HPO
- Optional test set evaluation when
verbose=TrueThe architecture is well-structured and follows ML best practices.
749-752: LGTM!Good defensive addition to handle class imbalance in cross-validation. Capping
num_cvat the minimum class count preventsGridSearchCVfrom failing when some classes have fewer samples than the number of folds.
759-840: LGTM!The
_get_prepared_training_datamethod correctly handles both RF and GNN data formats:
- Proper file path resolution based on
gnnflag- GNN data reconstructed from safetensors with label files
- Backward compatibility for legacy dict-based scores format
- Consistent train/test split for both paths
| # Open the json file save_mdl_path[:-4] + ".json" | ||
| with Path.open(path.with_suffix(".json"), encoding="utf-8") as f: | ||
| json_dict = json.load(f) |
There was a problem hiding this comment.
Incorrect usage of Path.open.
Path.open is an instance method, not a class method. The current code Path.open(path.with_suffix(".json"), ...) will fail because it passes the path as the first argument rather than calling the method on the Path instance.
🐛 Fix Path.open usage
- with Path.open(path.with_suffix(".json"), encoding="utf-8") as f:
+ with path.with_suffix(".json").open(encoding="utf-8") as f:
json_dict = json.load(f)🤖 Prompt for AI Agents
In @src/mqt/predictor/ml/predictor.py around lines 880 - 882, The code
incorrectly calls the instance method Path.open as a class method
(Path.open(path.with_suffix(".json"), ...)); change this to call open on the
Path instance: replace that line with
path.with_suffix(".json").open(encoding="utf-8") (or use the builtin
open(path.with_suffix(".json"), encoding="utf-8")) so json.load(f) reads from
the correctly opened file handle; the symbols to update are Path.open and
path.with_suffix(".json") where json_dict is assigned.
| labels_list = [distance_label] * n_circuits | ||
| training_data = TrainingData(X_train=feature_vector_list, y_train=labels_list) | ||
| labels = np.asarray(labels_list, dtype=np.float32) | ||
| if not gnn: | ||
| training_data = TrainingData(X_train=feature_vector_list, y_train=labels) | ||
| else: | ||
| training_data_list = [] | ||
| for i in range(n_circuits): | ||
| x, edge_idx, n_nodes = training_sample[i] | ||
| gnn_training_sample = Data( | ||
| x=x, | ||
| y=torch.tensor(labels_list[i], dtype=torch.float32), | ||
| edge_index=edge_idx, | ||
| num_nodes=n_nodes, | ||
| ) | ||
| training_data_list.append(gnn_training_sample) | ||
| training_data = TrainingData(X_train=training_data_list, y_train=labels) | ||
|
|
||
| # 3. Model Training | ||
| pred = ml_Predictor(figure_of_merit="hellinger_distance", devices=[device]) | ||
| trained_model = pred.train_random_forest_model(training_data) | ||
|
|
||
| assert np.isclose(trained_model.predict([feature_vector]), distance_label) | ||
|
|
||
|
|
||
| def test_train_and_qcompile_with_hellinger_model(source_path: Path, target_path: Path, device: Target) -> None: | ||
| """Test the entire predictor toolchain with the Hellinger distance model that was trained in the previous test.""" | ||
| pred = ml_Predictor(figure_of_merit="hellinger_distance", devices=[device], gnn=gnn) | ||
| if gnn: | ||
| trained_model = pred.train_gnn_model(training_data, num_epochs=200, patience=30, verbose=verbose) | ||
| else: | ||
| trained_model = pred.train_random_forest_model(training_data) | ||
|
|
||
| if not gnn: | ||
| assert np.isclose(trained_model.predict([feature_vector]), distance_label) | ||
| else: | ||
| trained_model.eval() | ||
| model_device = next(trained_model.parameters()).device | ||
| with torch.no_grad(): | ||
| batch = Batch.from_data_list(training_data.X_train).to(model_device) | ||
| out = trained_model(batch) | ||
| out = out.squeeze(-1) | ||
| predicted_values = out.cpu().numpy() | ||
| labels = np.asarray(labels_list, dtype=np.float32) | ||
| # it is set a tolerance value of 5e-1 just because of the small number of training samples | ||
| # for this reason we are not interested in a very accurate prediction here and a tolerance of 0.5 | ||
| # guarantees that the test passes even if the prediction is not very accurate | ||
| # (sometimes happens that the difference is higher than 0.3) | ||
| assert np.allclose(predicted_values, labels, atol=5e-1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable labels is shadowed, causing potential confusion.
The variable labels is assigned at line 187 as np.asarray(labels_list, dtype=np.float32), then reassigned at line 220 inside the GNN evaluation block. While this doesn't cause a bug (the inner assignment is intentional), it reduces readability. Consider using a different name like expected_labels at line 220.
Additionally, the high tolerance (atol=5e-1) is well-documented in the comment—this is acceptable for the small training set scenario.
♻️ Optional: rename inner variable for clarity
predicted_values = out.cpu().numpy()
- labels = np.asarray(labels_list, dtype=np.float32)
+ expected_labels = np.asarray(labels_list, dtype=np.float32)
# it is set a tolerance value of 5e-1 just because of the small number of training samples
# for this reason we are not interested in a very accurate prediction here and a tolerance of 0.5
# guarantees that the test passes even if the prediction is not very accurate
# (sometimes happens that the difference is higher than 0.3)
- assert np.allclose(predicted_values, labels, atol=5e-1)
+ assert np.allclose(predicted_values, expected_labels, atol=5e-1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| labels_list = [distance_label] * n_circuits | |
| training_data = TrainingData(X_train=feature_vector_list, y_train=labels_list) | |
| labels = np.asarray(labels_list, dtype=np.float32) | |
| if not gnn: | |
| training_data = TrainingData(X_train=feature_vector_list, y_train=labels) | |
| else: | |
| training_data_list = [] | |
| for i in range(n_circuits): | |
| x, edge_idx, n_nodes = training_sample[i] | |
| gnn_training_sample = Data( | |
| x=x, | |
| y=torch.tensor(labels_list[i], dtype=torch.float32), | |
| edge_index=edge_idx, | |
| num_nodes=n_nodes, | |
| ) | |
| training_data_list.append(gnn_training_sample) | |
| training_data = TrainingData(X_train=training_data_list, y_train=labels) | |
| # 3. Model Training | |
| pred = ml_Predictor(figure_of_merit="hellinger_distance", devices=[device]) | |
| trained_model = pred.train_random_forest_model(training_data) | |
| assert np.isclose(trained_model.predict([feature_vector]), distance_label) | |
| def test_train_and_qcompile_with_hellinger_model(source_path: Path, target_path: Path, device: Target) -> None: | |
| """Test the entire predictor toolchain with the Hellinger distance model that was trained in the previous test.""" | |
| pred = ml_Predictor(figure_of_merit="hellinger_distance", devices=[device], gnn=gnn) | |
| if gnn: | |
| trained_model = pred.train_gnn_model(training_data, num_epochs=200, patience=30, verbose=verbose) | |
| else: | |
| trained_model = pred.train_random_forest_model(training_data) | |
| if not gnn: | |
| assert np.isclose(trained_model.predict([feature_vector]), distance_label) | |
| else: | |
| trained_model.eval() | |
| model_device = next(trained_model.parameters()).device | |
| with torch.no_grad(): | |
| batch = Batch.from_data_list(training_data.X_train).to(model_device) | |
| out = trained_model(batch) | |
| out = out.squeeze(-1) | |
| predicted_values = out.cpu().numpy() | |
| labels = np.asarray(labels_list, dtype=np.float32) | |
| # it is set a tolerance value of 5e-1 just because of the small number of training samples | |
| # for this reason we are not interested in a very accurate prediction here and a tolerance of 0.5 | |
| # guarantees that the test passes even if the prediction is not very accurate | |
| # (sometimes happens that the difference is higher than 0.3) | |
| assert np.allclose(predicted_values, labels, atol=5e-1) | |
| labels_list = [distance_label] * n_circuits | |
| labels = np.asarray(labels_list, dtype=np.float32) | |
| if not gnn: | |
| training_data = TrainingData(X_train=feature_vector_list, y_train=labels) | |
| else: | |
| training_data_list = [] | |
| for i in range(n_circuits): | |
| x, edge_idx, n_nodes = training_sample[i] | |
| gnn_training_sample = Data( | |
| x=x, | |
| y=torch.tensor(labels_list[i], dtype=torch.float32), | |
| edge_index=edge_idx, | |
| num_nodes=n_nodes, | |
| ) | |
| training_data_list.append(gnn_training_sample) | |
| training_data = TrainingData(X_train=training_data_list, y_train=labels) | |
| # 3. Model Training | |
| pred = ml_Predictor(figure_of_merit="hellinger_distance", devices=[device], gnn=gnn) | |
| if gnn: | |
| trained_model = pred.train_gnn_model(training_data, num_epochs=200, patience=30, verbose=verbose) | |
| else: | |
| trained_model = pred.train_random_forest_model(training_data) | |
| if not gnn: | |
| assert np.isclose(trained_model.predict([feature_vector]), distance_label) | |
| else: | |
| trained_model.eval() | |
| model_device = next(trained_model.parameters()).device | |
| with torch.no_grad(): | |
| batch = Batch.from_data_list(training_data.X_train).to(model_device) | |
| out = trained_model(batch) | |
| out = out.squeeze(-1) | |
| predicted_values = out.cpu().numpy() | |
| expected_labels = np.asarray(labels_list, dtype=np.float32) | |
| # it is set a tolerance value of 5e-1 just because of the small number of training samples | |
| # for this reason we are not interested in a very accurate prediction here and a tolerance of 0.5 | |
| # guarantees that the test passes even if the prediction is not very accurate | |
| # (sometimes happens that the difference is higher than 0.3) | |
| assert np.allclose(predicted_values, expected_labels, atol=5e-1) |
🤖 Prompt for AI Agents
In @tests/hellinger_distance/test_estimated_hellinger_distance.py around lines
186 - 225, The variable labels is reassigned inside the GNN evaluation block
which shadows the outer labels; rename the inner variable (e.g.,
expected_labels) where you currently do labels = np.asarray(labels_list,
dtype=np.float32) and update subsequent references (the assert that compares
predicted_values to labels -> expected_labels) so the outer labels remain
unchanged and the intent is clearer; ensure you only change occurrences in the
GNN evaluation block (the scope around trained_model.eval(), model_device
retrieval, Batch.from_data_list(...) and the final np.allclose assertion).
| # check for controlling that the name starts with a number and ends with .safetensors asdefined in the predictor | ||
| assert any(f.is_file() and f.suffix == ".safetensors" and f.stem.isdigit() for f in dataset_dir.iterdir()) |
There was a problem hiding this comment.
Minor typo in comment.
The comment has a minor typo: "asdefined" should be "as defined".
📝 Fix typo
- # check for controlling that the name starts with a number and ends with .safetensors asdefined in the predictor
+ # check for controlling that the name starts with a number and ends with .safetensors as defined in the predictor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # check for controlling that the name starts with a number and ends with .safetensors asdefined in the predictor | |
| assert any(f.is_file() and f.suffix == ".safetensors" and f.stem.isdigit() for f in dataset_dir.iterdir()) | |
| # check for controlling that the name starts with a number and ends with .safetensors as defined in the predictor | |
| assert any(f.is_file() and f.suffix == ".safetensors" and f.stem.isdigit() for f in dataset_dir.iterdir()) |
🤖 Prompt for AI Agents
In @tests/hellinger_distance/test_estimated_hellinger_distance.py around lines
292 - 293, The inline comment above the assertion using
dataset_dir.iterdir()/f.suffix/f.stem.isdigit() contains a typo: change
"asdefined" to "as defined" so the comment reads that the name starts with a
number and ends with .safetensors "as defined" in the predictor; update only the
comment text near that assert in test_estimated_hellinger_distance.py.
2 participants
Description
This PR introduces a Graph Neural Network (GNN) as an alternative to the Random Forest model for predicting the best device to run a quantum circuit.
To support this, the preprocessing pipeline was redesigned: instead of manually extracting features from the circuit, the model now directly takes as input the Directed Acyclic Graph (DAG) representation of the quantum circuit.
🚀 Major Changes
Graph Neural Network Integration
🎯 Motivation
🔧 Fixes and Enhancements
📦 Dependency Updates
optuna>=4.5.0torch-geometric>=2.6.1Checklist: