Add Photonic Subcircuit Compiler - #1059
Open
tobi-forster wants to merge 62 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
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.
| # 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.
…unich-quantum-toolkit/qmap into tobi/add-photonics-subcircuit-compiler
…bcircuit-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
3 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.