Add Photonic Subcircuit Compiler - #1059
Open
tobi-forster wants to merge 48 commits into
Open
Conversation
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a photonic compiler package for routing and optimizing unitary subcircuits on MZI meshes, with Perceval simulation, baseline comparison, batch evaluation, hardware data, documentation, optional dependencies, and tests. ChangesPhotonic subcircuit compiler
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Compiler
participant RoutingGraph
participant PhaseOptimizer
participant Perceval
Caller->>Compiler: compile_subcircuit(...)
Compiler->>RoutingGraph: construct_graph(...)
RoutingGraph-->>Compiler: best route and ports
Compiler->>PhaseOptimizer: optimize routed phases
PhaseOptimizer-->>Compiler: CompilationResult
Caller->>Compiler: evaluate_subcircuit(...)
Compiler->>Perceval: simulate proposed and baseline chips
Perceval-->>Compiler: performance metrics
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eval/ph/subcircuit_compilation_data_collection.ipynb`:
- Around line 46-54: The OptimizationConfig constructor in the notebook still
passes unsupported kwargs, causing a TypeError at runtime. Update the config
setup to remove num_restarts and restart_perturbation, keeping the remaining
OptimizationConfig parameters unchanged. Use the existing OptimizationConfig
call in the subcircuit_compilation_data_collection notebook as the target for
this fix.
In `@python/mqt/qmap/ph/baseline.py`:
- Around line 21-38: The nested loops in embed_target_unitary_into_chip are
performing per-element tensor writes for the top-left block copy; replace them
with a single sliced block assignment on the embedded tensor using the
target_dim region. Keep the same behavior in embed_target_unitary_into_chip, but
use direct indexing into embedded with the target_unitary block so the function
is clearer and avoids Python-level iteration overhead.
- Around line 21-38: Add an input validation guard in
embed_target_unitary_into_chip before the nested loops: if target_dim is greater
than chip_dim, raise a ValueError instead of indexing into embedded. Keep the
fix localized to this helper so the torch.eye allocation and assignment loop
only run when the target block fits within the chip-sized tensor.
In `@python/mqt/qmap/ph/data_collection.py`:
- Line 77: The collect_pipeline_results function is too complex and should be
split up to satisfy Ruff PLR0912/PLR0915. Extract the hardware-cache
construction logic and the per-repeat run loop into separate helper functions,
then have collect_pipeline_results orchestrate those helpers while keeping its
responsibilities minimal. Use the existing collect_pipeline_results entry point
and any new private helpers to preserve behavior and make the control flow
easier to follow.
- Around line 216-224: The std value returned by `_mean_std` in
`data_collection.py` is never used because each call site only keeps the mean,
so the second return is dead weight. Update the aggregation logic around
`_mean_std` and the result assembly in `data_collection` to either include the
standard deviation in the collected outputs (and any downstream consumers) or
simplify `_mean_std` to return only the mean and adjust all call sites
accordingly.
- Around line 220-223: The aggregated loss values are computed in the data
collection flow but never make it into the final grouped result. Update the
aggregation in data_collection.py, especially in the logic around the rows
assembly and the groupby(...).agg(...) call in the aggregation step, so
`mean_loss` and `mean_baseline_loss` are actually included in the aggregated
output (or remove the unused computations if they are not meant to be returned).
Keep the `rows` construction and `df_aggregated` schema consistent so the loss
fields are not silently dropped.
- Around line 72-74: The helper _mean_std currently has no Google-style
docstring, so add one directly above the function definition describing its
purpose, the values argument, and the returned mean/std tuple in Google format.
Keep the implementation unchanged and ensure the docstring follows the existing
Python docstring guidelines used elsewhere in data_collection.py.
- Around line 84-87: Make the boolean flags in data_collection.py keyword-only
to avoid positional boolean arguments. Update the affected function signature
around the parameters input_losses, output_losses, and ideal_beam_splitters
(along with custom_bs_data if needed) by inserting a keyword-only separator so
callers must pass these flags by name. Check all call sites of the function to
ensure they use the explicit keywords.
- Around line 173-178: The seed in data_collection.py’s unitary generation logic
is derived from target_dim + unitary_index, which can repeat across different
sweep points and create correlated RNG streams. Update the seeding in the loop
that calls get_haar_random_unitary so each (target_dim, unitary_index) pair maps
to a unique deterministic seed, for example by combining both values with a
collision-resistant formula or hash. Keep the change localized around
unitary_seed and target_unitary so the sweep remains reproducible but
independent across target_dim values.
- Around line 247-266: Handle the empty rows case before calling groupby in
data_collection.py: when rows is empty, df will not contain the grouping columns
and df.groupby(groupby_cols, ...) will raise a KeyError. Update the logic around
the df and df_aggregated निर्माण so that an empty setups list or
num_unitaries_per_setup=0 returns an empty DataFrame with the expected output
schema instead of grouping. Use the existing groupby_cols and aggregation fields
in the same block to define the return shape consistently.
In `@python/mqt/qmap/ph/subcircuit_compilation.py`:
- Around line 86-307: compile_subcircuit is doing too many orchestration steps
in one place, which is why it exceeds the statement-count threshold. Refactor
the flow in subcircuit_compilation.py by extracting the routing/setup,
optimization, ideal-distribution computation, chip construction, and
simulation/evaluation logic into small helpers such as
_compute_ideal_distributions and _optimize_and_build_chip, while keeping
compile_subcircuit as the top-level coordinator. Preserve the existing behavior
and data flow through unique symbols like get_best_route,
optimize_unitary_subcircuit_parameters, create_mzi_chip, simulate_with_loss, and
evaluate_chip_performance, and keep RunResult assembly in the main function.
- Around line 238-267: The ideal Perceval simulation is being run twice with
identical inputs in the ground-truth section, duplicating an expensive
deterministic step. Reuse the single result from the
`ground_truth_processor`/`algorithm.Sampler(...).probs()` computation for both
`ideal_probability_distribution` and `baseline_ideal_probability_distribution`,
and remove the redundant `baseline_ground_truth_processor` setup while keeping
the existing `pcvl_u`, `ground_truth_processor`, and sampler flow intact.
In `@python/mqt/qmap/ph/unitary_to_phase_compilation.py`:
- Around line 519-609: The optimization loop in unitary_to_phase_compilation is
tracking best_loss but never stores the matching parameter state, so the return
value currently reflects the final iterate instead of the best one. Update the
loop around best_loss/no_improve_steps to snapshot phase_shifter_params whenever
a new best loss is found, and return that saved best tensor from the final
dictionary instead of the current detached phase_shifter_params. Use the
existing symbols best_loss, phase_shifter_params, and the function’s return
block to locate the change.
- Line 586: The progress-check in unitary-to-phase compilation is a dead no-op
because it only evaluates a boolean and discards it, so the verbose flag never
triggers logging. Update the progress reporting logic in the loop that uses
index and verbose so that it actually emits a message every 100 iterations when
verbose is enabled, using the surrounding compilation method/function as the
place to hook the logging behavior.
In `@test/python/ph/conftest.py`:
- Around line 21-33: The current `conftest.py` setup in the `mqt`/`mqt.qmap`
import shim mutates `sys.modules` globally, which can leak the stubs into later
tests in the same interpreter. Scope the stub registration in the loop that
creates the `ModuleType` entries to the photonics tests by using a fixture or
adding teardown logic in `conftest.py` to remove or restore the `mqt` and
`mqt.qmap` modules after the tests finish, while keeping `sys.path.insert`
limited to the test session setup.
In `@test/python/ph/test_baseline.py`:
- Around line 23-118: Add explicit -> None return annotations to every flagged
staticmethod test in TestGetBaselineActiveCols, TestEmbedTargetUnitaryIntoChip,
and TestGetBaselineInputPorts (and any other test methods in test_baseline.py
that currently omit them) to satisfy Ruff ANN205; update the function signatures
directly on each test_* method rather than suppressing the warning.
In `@test/python/ph/test_data_collection.py`:
- Around line 55-62: The comment in test_valid_multiple_setups is inaccurate
about why (6,6) is not produced by build_setup_grid; update the test note to
reflect that target_dims_list only includes 2 and 4, so 6 is never a candidate
target dimension. Keep the assertions as-is and revise the explanatory text in
test_valid_multiple_setups to match the actual inputs and expected Cartesian
product.
In `@test/python/ph/test_graph.py`:
- Around line 26-212: Add explicit `-> None` return annotations to every
`@staticmethod` test in `TestBarFidelity`, `TestCrossFidelity`,
`TestGenerateBeamSplitterMatrix`, `TestDetermineRoutingFidelities`, and
`TestConstructGraph` in `test_graph.py`. The issue is Ruff ANN205 missing return
type annotations on these test methods. Update each test method signature to
include `-> None` so the file stays lint-clean without suppressions.
In `@test/python/ph/test_phases.py`:
- Around line 24-128: Annotate the test methods in TestPhases and
TestGetEffectiveParamsAndMask with explicit -> None return types to satisfy
ANN205, and add explicit torch.Tensor return annotations to the helper methods
_bar_mask and _cross_mask so the type warnings are cleared without suppressing
them.
In `@test/python/ph/test_routing.py`:
- Around line 30-232: The test methods in TestRouting and the related static
test classes are missing explicit return type annotations, triggering ANN205
warnings. Update each `@staticmethod` test method in this file to declare -> None,
including methods like test_straight_route_first_position,
test_first_mode_active, test_straight_route_chip4_target2, and the other test_*
functions, without changing the test logic or assertions.
In `@test/python/ph/test_subcircuit_compilation.py`:
- Around line 167-262: Add explicit `-> None` return annotations to each
`@staticmethod` test in `TestRunResultProperties` and `TestRunValueRanges` to
satisfy Ruff ANN205. Update the signatures of the test methods like
`test_returns_run_result`, `test_performance_dict_has_required_keys`, and the
other static test helpers in this file so they clearly return nothing; keep the
test logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb9671aa-8b70-45d2-a4c2-f01ce933bcdc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.license-tools-config.jsoneval/ph/subcircuit_compilation_data_collection.ipynbnoxfile.pypyproject.tomlpython/mqt/qmap/ph/__init__.pypython/mqt/qmap/ph/baseline.pypython/mqt/qmap/ph/data_collection.pypython/mqt/qmap/ph/graph.pypython/mqt/qmap/ph/perceval_simulation.pypython/mqt/qmap/ph/routing.pypython/mqt/qmap/ph/routing_to_phases.pypython/mqt/qmap/ph/subcircuit_compilation.pypython/mqt/qmap/ph/unitary_to_phase_compilation.pytest/python/ph/conftest.pytest/python/ph/test_baseline.pytest/python/ph/test_data_collection.pytest/python/ph/test_graph.pytest/python/ph/test_phases.pytest/python/ph/test_routing.pytest/python/ph/test_subcircuit_compilation.py
tobi-forster
force-pushed
the
tobi/add-photonics-subcircuit-compiler
branch
from
July 6, 2026 14:51
ef3a439 to
0313d46
Compare
…mpiler' into tobi/add-photonics-subcircuit-compiler
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
278-281: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the missing PR reference definitions.
CHANGELOG.md#L278-L281: add[#1059]and restore[#1069]in the PR-link section.CHANGELOG.md#L21-L24: retain both references once their definitions exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 278 - 281, In the PR-link definitions section near lines 278-281 of CHANGELOG.md, add the missing [`#1059`] definition and restore [`#1069`] with their corresponding PR URLs. In the references near lines 21-24, retain both [`#1059`] and [`#1069`] entries once the definitions are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eval/ph/data_collection.py`:
- Around line 228-247: Update the repeat loop around evaluate_subcircuit to pass
a deterministic phase_noise_seed derived from unitary_seed and repeat_idx,
ensuring each benchmark repeat reproduces its phase-noise realization while
remaining distinct across repeats. Preserve the existing torch.manual_seed
formula and evaluation arguments.
In `@python/mqt/qmap/ph/graph.py`:
- Around line 50-54: Make the new Boolean parameters keyword-only by inserting a
positional-argument separator in generate_beam_splitter_matrix at
python/mqt/qmap/ph/graph.py lines 50-54 before ideal_bs, in the routing function
at python/mqt/qmap/ph/routing_to_phases.py lines 68-73 before
optimize_routing_parameters, and in the function at
python/mqt/qmap/ph/routing_to_phases.py lines 183-187 before
exclude_edge_phase_shifters; preserve existing parameter behavior while
preventing ambiguous positional calls and Ruff FBT001/FBT002 warnings.
- Around line 241-243: Update the fidelity lookup helper covering the
movement-mask processing around graph layers 2–4 so an edge starting at graph
layer L reads fidelities from the preceding chip layer, L - 1, rather than L +
1. Preserve the existing layer and mode-pair mapping, and add a non-ideal
regression test with distinct per-layer fidelities that verifies shortest-path
selection uses the preceding routing layer.
In `@python/mqt/qmap/ph/perceval_simulation.py`:
- Around line 224-236: The simulation result should return the normalized
conditional distribution rather than the raw corrected weights. In the flow that
computes norm_sim, update the returned mapped_distribution field to use norm_sim
when compensated_weight_sum is positive, while preserving the existing empty or
zero-weight behavior.
In `@python/mqt/qmap/ph/routing_to_phases.py`:
- Around line 51-63: Update the MZI refinement logic around the mzi_pairs loop
to determine absent or virtual shifters from the movement_mask states rather
than embedded_params phase magnitudes. Preserve MaskState.MZI for valid pairs,
including pairs whose phases are zero or near zero, and ensure both MZI cells
remain trainable instead of being reclassified or having gradients zeroed in the
related routing logic.
In `@python/mqt/qmap/ph/subcircuit_compilation.py`:
- Around line 459-496: Update the paired create_mzi_chip calls in the
compilation flow to generate one phase-error offset matrix and pass that same
realization to both virtual_chip and baseline_virtual_chip. Preserve the
existing phase_noise_seed behavior and ensure the shared offsets are applied
consistently so only circuit placement and optimization differ.
- Around line 352-353: Validate target_dim immediately after it is derived from
target_unitary in the subcircuit compilation flow: require it to be positive,
even, and no greater than chip_dim, and raise a clear error when invalid. Keep
routing and Perceval input-state construction unchanged for valid dimensions.
In `@python/mqt/qmap/ph/unitary_to_phase_compilation.py`:
- Around line 616-621: The unitary compilation result must expose the loss
associated with best_params, and both subcircuit result paths must consume it
instead of the final history loss. In
python/mqt/qmap/ph/unitary_to_phase_compilation.py lines 616-621, add best_loss
to the returned mapping; in python/mqt/qmap/ph/subcircuit_compilation.py lines
219-220 and 276-277, update the proposed and baseline result construction to use
the returned best_loss.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 278-281: In the PR-link definitions section near lines 278-281 of
CHANGELOG.md, add the missing [`#1059`] definition and restore [`#1069`] with their
corresponding PR URLs. In the references near lines 21-24, retain both [`#1059`]
and [`#1069`] entries once the definitions are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c65fbd72-f7be-4d73-b2ce-c5db4f118ea4
⛔ Files ignored due to path filters (2)
eval/ph/results/pipeline_results.csvis excluded by!**/*.csvuv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.license-tools-config.json.pre-commit-config.yamlCHANGELOG.mddocs/ph_subcircuit_compiler.mdeval/ph/data_collection.pyeval/ph/hardware_data/24_mode_input_transmissions.txteval/ph/hardware_data/24_mode_output_transmissions.txteval/ph/hardware_data/48_mode_input_transmissions.txteval/ph/hardware_data/48_mode_output_transmissions.txteval/ph/subcircuit_compilation_data_collection.ipynbnoxfile.pypyproject.tomlpython/mqt/qmap/ph/__init__.pypython/mqt/qmap/ph/baseline.pypython/mqt/qmap/ph/graph.pypython/mqt/qmap/ph/perceval_simulation.pypython/mqt/qmap/ph/routing.pypython/mqt/qmap/ph/routing_to_phases.pypython/mqt/qmap/ph/subcircuit_compilation.pypython/mqt/qmap/ph/unitary_to_phase_compilation.pytest/python/ph/conftest.pytest/python/ph/test_baseline.pytest/python/ph/test_data_collection.pytest/python/ph/test_graph.pytest/python/ph/test_phases.pytest/python/ph/test_routing.pytest/python/ph/test_subcircuit_compilation.pytest/python/ph/test_unitary_to_phase_compilation.py
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
tobi-forster
added
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
python/mqt/qmap/ph/unitary_to_phase_compilation.py (1)
601-601: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDead statement —
verboseprogress logging still has no effect (unresolved from a prior review).
verbose and index % 100 == 0evaluates a boolean and discards it. Theverboseparameter (documented at Line 450 as "print progress every 100 iterations") has no observable effect anywhere in the function.🐛 Proposed fix
- verbose and index % 100 == 0 + if verbose and index % 100 == 0: + print(f"Iteration {index}: loss={loop_loss:.6e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/mqt/qmap/ph/unitary_to_phase_compilation.py` at line 601, Replace the discarded boolean expression in the unitary-to-phase compilation loop with an observable progress action: when verbose is enabled and index is divisible by 100, print the documented progress information. Remove the standalone `verbose and index % 100 == 0` statement and preserve the existing iteration behavior otherwise.python/mqt/qmap/ph/subcircuit_compilation.py (1)
354-355: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
target_dimbefore use (still open).
target_dimis derived fromtarget_unitary.shape[0]in bothcompile_subcircuit(lines 354-355) andevaluate_subcircuit(lines 459-460) without checking it's positive, even, and ≤chip_dim. Invalid values (odd, non-positive, or larger thanchip_dim) will fail later in routing/Perceval input-state construction with opaque errors rather than a clear message.Proposed validation
chip_dim = len(input_transmissions) target_dim = int(target_unitary.shape[0]) + if target_dim <= 0 or target_dim % 2 != 0: + msg = "target_unitary dimension must be a positive even number for dual-rail encoding." + raise ValueError(msg) + if target_dim > chip_dim: + msg = "target_unitary dimension cannot exceed the chip dimension." + raise ValueError(msg)Apply the same check in both
compile_subcircuitandevaluate_subcircuit(or factor into a shared helper).Also applies to: 459-460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/mqt/qmap/ph/subcircuit_compilation.py` around lines 354 - 355, Validate target_dim immediately after it is derived in both compile_subcircuit and evaluate_subcircuit, requiring it to be positive, even, and no greater than chip_dim; raise a clear validation error before routing or input-state construction when invalid. Reuse a shared helper if appropriate, while preserving valid execution paths.python/mqt/qmap/ph/baseline.py (1)
21-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing guard for
target_dim > chip_dimstill absent.A prior review already asked for a
ValueErrorguard here (marked resolved), but the current code still has none. Withtarget_dim > chip_dim,embedded[:target_dim, :target_dim]silently clips toembedded's real shape, and assigning the largertarget_unitaryraises an opaque PyTorch shape-mismatch error instead of a clear validation message.🛡️ Proposed fix
def embed_target_unitary_into_chip(target_unitary: np.ndarray, chip_dim: int, target_dim: int) -> torch.Tensor: """Embed a target unitary into the top-left block of a chip-sized identity matrix. ... """ + if target_dim > chip_dim: + msg = f"target_dim ({target_dim}) cannot exceed chip_dim ({chip_dim})." + raise ValueError(msg) embedded = torch.eye(chip_dim, dtype=torch.complex128) embedded[:target_dim, :target_dim] = torch.tensor(target_unitary, dtype=torch.complex128) return embedded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/mqt/qmap/ph/baseline.py` around lines 21 - 36, Add an explicit ValueError guard at the start of embed_target_unitary_into_chip when target_dim exceeds chip_dim, before constructing or assigning to the embedded tensor, with a clear message describing the invalid dimensions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 21-22: Synchronize the changelog entry references with the PR
links definitions: ensure the new [`#1059`] reference has a matching definition
and restore or update the definition for [`#1069`] so all references in the
changelog resolve correctly. Keep the existing PR link formatting and author
attribution conventions unchanged.
In `@docs/ph_subcircuit_compiler.md`:
- Around line 24-26: Update the coincidence rate definition in the documentation
paragraph to describe the probability that all photons are detected
simultaneously in the computation zone, replacing the reference to correct
output ports while leaving the separate TVD metric distinction intact.
- Around line 87-90: Correct the calibration sentence in the documentation by
replacing the ungrammatical phrase “a calibration measurements” with
“calibration measurements” or “a calibration measurement,” while preserving the
surrounding meaning.
In `@python/mqt/qmap/ph/graph.py`:
- Around line 50-153: Refactor generate_beam_splitter_matrix and construct_graph
into focused named helpers to reduce their PLR0912/PLR0915 complexity. Extract
beam-splitter statistics generation, normalization, and pair assembly from
generate_beam_splitter_matrix, and extract layer indexing and edge construction
from construct_graph, preserving the existing ordering, parity guards, and
output behavior.
- Around line 406-424: The construct_graph validation must reject odd chip_dim -
target_dim values before layer sizing occurs. Add a ValueError guard alongside
the existing target_dim checks, update the Raises documentation, and add a
regression test confirming construct_graph(3, 2) raises ValueError.
In `@python/mqt/qmap/ph/perceval_simulation.py`:
- Around line 23-30: Make the boolean parameters keyword-only in both
create_mzi_chip and the related function containing
apply_output_transmission_correction by inserting a keyword-only separator
before those flags. Preserve existing parameter behavior and ensure their call
sites continue passing them by keyword.
In `@python/mqt/qmap/ph/routing_to_phases.py`:
- Around line 117-126: Remove the near-zero phase heuristic in the state ==
MaskState.MZI branch of routing_to_phases: do not overwrite raw_params with (0,
π) or zero grad_mask for compute-zone MZIs based on phase values. Preserve
legitimate (0, 0) phases and their gradients, relying on the structural
movement_mask state to distinguish routing from compute zones; add regression
coverage for a compute MZI with both phases near zero if the existing test suite
supports it.
In `@python/mqt/qmap/ph/routing.py`:
- Around line 279-303: Update the later-layer transition handling in
route_to_movement_mask so both the i % 2 == 0 and i % 2 == 1 branches validate
that route[i - 1] is the expected adjacent node before marking CROSS, raising
the same invalid-edge ValueError behavior used by the i == 2 branch for
non-adjacent transitions. Add or update route_to_movement_mask tests in
TestRouteToMovementMask to cover invalid later-layer transitions and verify
target_dim-wide CROSS slices for target_dim values larger than 2.
---
Duplicate comments:
In `@python/mqt/qmap/ph/baseline.py`:
- Around line 21-36: Add an explicit ValueError guard at the start of
embed_target_unitary_into_chip when target_dim exceeds chip_dim, before
constructing or assigning to the embedded tensor, with a clear message
describing the invalid dimensions.
In `@python/mqt/qmap/ph/subcircuit_compilation.py`:
- Around line 354-355: Validate target_dim immediately after it is derived in
both compile_subcircuit and evaluate_subcircuit, requiring it to be positive,
even, and no greater than chip_dim; raise a clear validation error before
routing or input-state construction when invalid. Reuse a shared helper if
appropriate, while preserving valid execution paths.
In `@python/mqt/qmap/ph/unitary_to_phase_compilation.py`:
- Line 601: Replace the discarded boolean expression in the unitary-to-phase
compilation loop with an observable progress action: when verbose is enabled and
index is divisible by 100, print the documented progress information. Remove the
standalone `verbose and index % 100 == 0` statement and preserve the existing
iteration behavior otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83008b97-9eb4-483f-89f0-525db597af48
⛔ Files ignored due to path filters (2)
eval/ph/results/pipeline_results.csvis excluded by!**/*.csvuv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.license-tools-config.json.pre-commit-config.yamlCHANGELOG.mddocs/ph_subcircuit_compiler.mdeval/ph/data_collection.pyeval/ph/hardware_data/24_mode_input_transmissions.txteval/ph/hardware_data/24_mode_output_transmissions.txteval/ph/hardware_data/48_mode_input_transmissions.txteval/ph/hardware_data/48_mode_output_transmissions.txteval/ph/subcircuit_compilation_data_collection.ipynbnoxfile.pypyproject.tomlpython/mqt/qmap/ph/__init__.pypython/mqt/qmap/ph/baseline.pypython/mqt/qmap/ph/graph.pypython/mqt/qmap/ph/perceval_simulation.pypython/mqt/qmap/ph/routing.pypython/mqt/qmap/ph/routing_to_phases.pypython/mqt/qmap/ph/subcircuit_compilation.pypython/mqt/qmap/ph/unitary_to_phase_compilation.pytest/python/ph/conftest.pytest/python/ph/test_baseline.pytest/python/ph/test_data_collection.pytest/python/ph/test_graph.pytest/python/ph/test_phases.pytest/python/ph/test_routing.pytest/python/ph/test_subcircuit_compilation.pytest/python/ph/test_unitary_to_phase_compilation.py
Collaborator
|
@coderabbitai There are some unresolved comments. Please, check whether they were already tackled and then resolve them, otherwise comment, why they are not resolved yet. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api graphql \
-F owner='munich-quantum-toolkit' \
-F name='qmap' \
-F number=1059 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
originalLine
comments(first: 20) {
nodes {
id
author { login }
body
createdAt
url
}
}
}
}
}
}
}' > /tmp/pr-1059-review-threads.json
jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| "THREAD \(.id)\nPATH \(.path // "general") LINE \(.line // .originalLine // 0) OUTDATED \(.isOutdated)\n"
+ (.comments.nodes[] | "AUTHOR \(.author.login)\nURL \(.url)\nBODY\n\(.body)\n")
+ "----"
' /tmp/pr-1059-review-threads.jsonLength of output: 22494 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Structure =='
ast-grep outline eval/ph/data_collection.py --items all --view expanded
ast-grep outline python/mqt/qmap/ph/unitary_to_phase_compilation.py --items all --match 'optimize_unitary_subcircuit_parameters' --view expanded
ast-grep outline test/python/ph/conftest.py --items all --view expanded
ast-grep outline python/mqt/qmap/ph/routing_to_phases.py --items all --view expanded
printf '%s\n' '== Current relevant code =='
nl -ba eval/ph/data_collection.py | sed -n '1,330p'
nl -ba python/mqt/qmap/ph/unitary_to_phase_compilation.py | sed -n '500,660p'
nl -ba test/python/ph/conftest.py | sed -n '1,180p'
nl -ba python/mqt/qmap/ph/routing_to_phases.py | sed -n '1,180p'
printf '%s\n' '== Targeted test/usages search =='
rg -n -C 4 --glob '*.py' \
'unitary_seed|base_seed|avg_loss|avg_baseline_loss|std|empty.*DataFrame|num_unitaries_per_setup|verbose|capsys|sys\.modules|monkeypatch|priority_map|mixed.*(?:BAR|CROSS)|MaskState\.(?:BAR|CROSS)' \
eval/ph test/python/ph python/mqt/qmap/phLength of output: 3064 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== eval/ph/data_collection.py =='
sed -n '70,460p' eval/ph/data_collection.py
printf '%s\n' '== optimizer loop =='
sed -n '400,660p' python/mqt/qmap/ph/unitary_to_phase_compilation.py
printf '%s\n' '== photonics conftest =='
sed -n '1,180p' test/python/ph/conftest.py
printf '%s\n' '== routing-to-phases =='
sed -n '1,190p' python/mqt/qmap/ph/routing_to_phases.py
printf '%s\n' '== Targeted tests/usages =='
rg -n -C 4 --glob '*.py' \
'unitary_seed|base_seed|avg_loss|avg_baseline_loss|_mean\(|num_unitaries_per_setup|verbose|capsys|sys\.modules|priority_map|MaskState\.(BAR|CROSS)' \
eval/ph test/python/ph python/mqt/qmap/ph || trueLength of output: 62874 🏁 Script executed: #!/bin/bash
set -euo pipefail
resolve_thread() {
local thread_id="$1"
gh api graphql \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' \
-F threadId="$thread_id" \
--jq '.data.resolveReviewThread.thread | "\(.id) resolved=\(.isResolved)"'
}
reply_thread() {
local thread_id="$1"
local body="$2"
gh api graphql \
-f query='
mutation($threadId: ID!, $body: String!) {
addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: $threadId, body: $body}) {
comment { url body }
}
}' \
-F threadId="$thread_id" \
-F body="$body" \
--jq '.data.addPullRequestReviewThreadReply.comment.url'
}
# Addressed in the current branch.
resolve_thread 'PRRT_kwDOEgwI3c6OOnim'
resolve_thread 'PRRT_kwDOEgwI3c6OOnir'
resolve_thread 'PRRT_kwDOEgwI3c6OOnis'
resolve_thread 'PRRT_kwDOEgwI3c6OOniv'
resolve_thread 'PRRT_kwDOEgwI3c6QbIif'
# Still valid in the current branch; keep open and explain why.
reply_thread \
'PRRT_kwDOEgwI3c6OOni2' \
'Checked the current branch: this is not resolved. `optimize_unitary_subcircuit_parameters()` still evaluates `verbose and index % 100 == 0` without emitting output, while its docstring promises progress logging. The thread remains open until the condition performs logging (or the unused `verbose` API is removed and documented accordingly).'
reply_thread \
'PRRT_kwDOEgwI3c6OOni9' \
'Checked the current branch: this is not resolved. `test/python/ph/conftest.py` still registers synthetic `mqt` and `mqt.qmap` modules in `sys.modules` during conftest import and does not restore them. Those stubs can therefore remain visible to other tests in the same pytest process. The thread remains open pending scoped setup/teardown or an import strategy that does not mutate process-global module state.'Length of output: 2342
Resolved five that are addressed in the current branch:
Kept two open and replied in their threads:
|
ystade
marked this pull request as ready for review
July 20, 2026 11:53
ystade
requested changes
Jul 20, 2026
ystade
left a comment
ystade
left a comment
Collaborator
There was a problem hiding this comment.
@tobi-forster I am very happy to see this kick-start of a compiler for photonics. Really, great work that resulted in quite some lines. Hence, I do not recall in detail anymore what all my comments were about but I do recall that there was nothing critical and mostly the comments are more of a remark. Feel free to ask any questions if anything is unclear and (opposed to the coding guidelines) please resolve every comment that is fixed. Otherwise I loose the overview.
Regarding the tests that are skipped: These are skipped because this PR does not touch the C++ code base, so this is all fine.
| # the torch-free tests (e.g. test_graph.py) still run. Remove this block and | ||
| # restore the unconditional `"--extra", "photonics"` args below once torch | ||
| # publishes macOS x86_64 wheels again or Intel macOS runners leave the matrix. | ||
| photonics_extra: tuple[str, ...] = ("--extra", "photonics") |
Collaborator
There was a problem hiding this comment.
I am wondering whether photonics_extra there is a more meaningful name for this variable. (1) one could imagine to list other extensions than photonics in the future; (2) that is an entirely "me"-problem: I did not know that these extras officially called extras since I haven't worked with Python so much recently. However, sometimes, they are also called optional_dependencies, which in my opinion is a more expressive variable name and better suited here.
| # extra there; the photonics tests self-skip via `pytest.importorskip`, and | ||
| # the torch-free tests (e.g. test_graph.py) still run. Remove this block and | ||
| # restore the unconditional `"--extra", "photonics"` args below once torch | ||
| # publishes macOS x86_64 wheels again or Intel macOS runners leave the matrix. |
Collaborator
There was a problem hiding this comment.
@burgholzer For me this solution is fine, just tagging you here for awareness when you review it.
Comment on lines
+21
to
+33
| _PYTHON_SRC = Path(__file__).parents[3] / "python" | ||
|
|
||
| for _pkg, _path in [ | ||
| ("mqt", _PYTHON_SRC / "mqt"), | ||
| ("mqt.qmap", _PYTHON_SRC / "mqt" / "qmap"), | ||
| ]: | ||
| if _pkg not in sys.modules: | ||
| _mod = ModuleType(_pkg) | ||
| _mod.__path__ = [str(_path)] | ||
| _mod.__package__ = _pkg | ||
| sys.modules[_pkg] = _mod | ||
|
|
||
| sys.path.insert(0, str(_PYTHON_SRC)) |
Collaborator
There was a problem hiding this comment.
@coderabbitai @tobi-forster Is it actually really necessary to stub mqt and qmt.qmap. Yes, indeed, the photonics compiler does not require a C++ build but we are building the C++ code anyways in the CI so I di not understand what one gains through stubbing it.
| assert torch.all(mask[4:8, 2] == MaskState.BAR) | ||
|
|
||
|
|
||
| class TestGetBestRoute: |
Collaborator
There was a problem hiding this comment.
I do not know where to comment that, I'll just do it here. All the tests form a very sophisticated test suite. Most of them test small details of the implementation (which is fine and also necessary). However, I am missing (or I have overlooked it) an end-to-end test that feeds in a certain phase matrix (or multiple to test multiple scenarios) and tests where the outcome is as expected. To this end, the input phase matrix should deterministically lead to one certain output.
This is more of a fundamental comment: In C++ one has a better separation of the public interface and functions that are only called internally. While it is quite useful to also test the internal functions from time to time, to verify that the internals work as expected, most of the test should through the user facing interface, i.e., calling the functions a user would call directly with a certain input and then verifying that the output is as expected. This allows in particular, to change the internals completely as long as the public facing interface remains the same.
…unich-quantum-toolkit/qmap into tobi/add-photonics-subcircuit-compiler
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces mqt.qmap.ph, a new Python subpackage implementing a photonic subcircuit compiler for MZI-mesh chips. The compiler routes photons to the computation zone by finding the lowest-loss path through the chip's MZI mesh, then fits the phase-shifter parameters to a target unitary via PyTorch gradient descent. The compiled circuit is evaluated using Perceval simulation, reporting coincidence rate and TVD against the ideal output distribution. A fixed-placement baseline is computed alongside every run to verify that routing improves performance under transmission loss.
Required dependencies:
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.