Update graphqomb compatibility

[masa10-f]

Summary

• Update the graphqomb dependency floor to 0.3.1
• Adapt compile_canvas_to_stim() to graphqomb's noise model and logical observable APIs
• Delay same-time flow-dependent measurements before scheduler validation
• Pin the CI ruff version so ruff-action uses the locally validated version instead of latest
• Add a regression test for scheduler causality adjustment

Root Cause

Graphqomb 0.3.x removed legacy stim_compile() noise arguments, moved logical observable handling into qompile(), and now validates manually provided schedulers before pattern generation. Existing ls-pattern schedules could contain flow-dependent measurements in the same time slice, which graphqomb now rejects.

The ruff workflow also passed requirements-dev.txt as a version source, but the file did not contain a parseable ruff version. GitHub Actions therefore installed latest ruff, which introduced failures unrelated to this change.

Validation

• uv run --no-sync pytest
• uv run --no-sync mypy
• uv run --no-sync pyright
• uv run --no-sync ruff check ./lspattern ./tests ./examples
• uv run --no-sync ruff format --check --diff
• PYTHONPATH=/home/masa10/git-repos/utftbmqc/graphqomb .venv/bin/python -m pytest -q

[claude]

Code Review: Update graphqomb compatibility

Overview

This PR adapts ls-pattern-compile to the graphqomb 0.3.x API, which made three breaking changes: moved noise arguments out of stim_compile() into dedicated NoiseModel objects, moved logical observable handling into qompile(), and added pre-validation of manually provided schedulers. The core new work is _delay_measurements_for_flow(), which repairs schedules where flow-dependent measurements land in the same time slice.

---

Correctness ✅

The approach is sound. Using TopologicalSorter for a single forward pass guarantees that by the time a node is processed, all its predecessors have already been adjusted. This makes a single O(N+E) pass sufficient — no iterative relaxation needed.

for node in TopologicalSorter(predecessors).static_order():
    ...
    for predecessor in predecessors[node]:
        predecessor_time = scheduler.measure_time[predecessor]
        if predecessor_time is not None and predecessor_time >= min_measure_time:
            min_measure_time = predecessor_time + 1

The logic computes max(original_time, max(pred_time + 1)) correctly. The in-place mutation propagates adjustments to subsequent nodes in topological order.

---

Issues & Suggestions

Minor: Misleading variable name

min_measure_time is initialized to the node's current time and then maximized upward. The name implies a minimum, but functionally it accumulates a lower bound (earliest allowed time). Consider renaming:

# Before
min_measure_time = measure_time
...
scheduler.measure_time[node] = min_measure_time

# After
earliest_allowed = measure_time
...
scheduler.measure_time[node] = earliest_allowed

Minor: Test placement

The new unit test test_delay_measurements_for_flow_splits_equal_time_dependencies tests a scheduler-level helper, but it's placed in test_graph_block_yaml.py. A file like test_compiler.py (or test_scheduler.py) would be a more natural home for it. Not blocking, but it reduces discoverability.

Suggestion: Add a cascading-delay test

The current test covers a 2-node chain (A→B at same time). A 3-node chain (A→B→C all at time 1, expected to become 1, 2, 3) would verify that topological ordering propagates cascading bumps correctly:

def test_delay_measurements_cascades() -> None:
    # A -> B -> C all at time=1; expected: A=1, B=2, C=3
    ...

This is especially worth having since the topological-sort propagation is the key insight of the fix.

Observation: requirements.txt floor only

graphqomb>=0.3.1 has no upper bound. This is consistent with every other dependency in the file, so no action required — just noting it as a future consideration if graphqomb 0.4.x introduces additional breaking changes.

---

API Migration ✅

The three API adaptations look correct:

Old	New
stim_compile(pattern, logical_observables_nodes, p_depol_after_clifford=..., p_before_meas_flip=...)	qompile(..., logical_observables=...) + stim_compile(pattern, noise_models=[...])
Inline noise kwargs	DepolarizingNoiseModel(p1=...), MeasurementFlipNoiseModel(p=...)
No pre-validation	scheduler.validate_schedule() called explicitly

---

Test Coverage ✅

• The new regression test is well-structured: it demonstrates the pre-fix failure (ValueError: DAG violation), applies the fix, then asserts both the expected values and that validate_schedule() now passes.
• The existing integration test test_load_canvas_with_graph_block exercises the full pipeline end-to-end including OBSERVABLE_INCLUDE, which catches any regression in the observable-handling migration.

---

Summary

The implementation is correct and the PR description clearly explains the root cause. The main actionable items are:

1. Rename min_measure_time → earliest_allowed (or similar) for readability.
2. Add a cascading test case (3-node chain) to verify propagation.
3. (Optional) Move the unit test to a more appropriate test file.

Otherwise this is a clean, focused migration. No security or performance concerns.

[claude]

Code Review — Update graphqomb compatibility

Overview

This PR adapts ls-pattern-compile to the breaking API changes in graphqomb 0.3.x: noise arguments moved out of stim_compile(), logical observables moved into qompile(), and the scheduler now validates before pattern generation. The key addition is _delay_measurements_for_flow() which fixes schedules that have flow-dependent measurements in the same time slice.

The change is well-scoped and the overall approach is sound. A few points worth discussing below.

---

_delay_measurements_for_flow — correctness

The topological-sort approach is correct: processing nodes in dependency order ensures that when a node's required time is bumped, its successors see the updated value immediately. The multi-hop case (A → B → C all at time 1) is handled correctly.

Naming nit: min_measure_time reads as "the minimum across predecessors", but it is actually "the adjusted required time for this node". A name like adjusted_time or required_time would be clearer.

Simplification opportunity: The loop body is slightly harder to follow than necessary because the threshold changes during iteration:

# current
min_measure_time = measure_time
for predecessor in predecessors[node]:
    predecessor_time = scheduler.measure_time[predecessor]
    if predecessor_time is not None and predecessor_time >= min_measure_time:
        min_measure_time = predecessor_time + 1
scheduler.measure_time[node] = min_measure_time

This is equivalent to — and arguably clearer as:

max_pred_time = max(
    (t for pred in predecessors[node]
       if (t := scheduler.measure_time[pred]) is not None),
    default=-1,
)
scheduler.measure_time[node] = max(measure_time, max_pred_time + 1)

Not a blocker, but worth considering for readability.

---

compile_canvas_to_stim — API migration

Moving logical_observables from stim_compile() into qompile() and converting the noise arguments to DepolarizingNoiseModel/MeasurementFlipNoiseModel objects looks like a straightforward 1-to-1 mapping of the new API. The insertion of _delay_measurements_for_flow + validate_schedule() before qompile() is in the right place — validation must happen after the fix is applied.

One blank-line artifact in the resulting file: after the detectors loop there are two consecutive blank lines before # extract logical observables from canvas. Minor style issue but worth a pass with ruff format.

---

Test coverage

Positives:

• The test correctly verifies that validate_schedule() raises before the fix and passes after, which is the right regression structure.
• Asserting specific time values (== 1, == 2) pins the exact behaviour.

Gaps to consider:

Case	Status
Linear conflict (input→middle at same time)	✅ covered
Already-valid schedule — function should be a no-op	❌ not tested
Multi-predecessor convergence (two sources conflict with one target)	❌ not tested
Chain of length > 2 (cascading bumps)	❌ not tested

The already-valid / idempotency case is particularly worth adding since it guards against the fix accidentally advancing times that did not need adjustment.

Private function import: The test imports _delay_measurements_for_flow by name. This is acceptable for internal unit testing but ties the test to the current module layout. If the function is later inlined or renamed, the test will break loudly rather than silently — which is actually fine here.

---

requirements-dev.txt — ruff pin

-ruff
+ruff==0.15.13

Pinning a linting tool is a semantic change: it trades reproducibility for upgrade friction. A few questions:

1. Is the pin intentional (CI reproducibility) or incidental (the version that happened to be installed locally)?
2. Is 0.15.13 the version that was tested against in the validation steps listed in the PR body?
3. If pinning is intentional, it is more conventional to manage tool versions via uv.lock or a [tool.uv] override in pyproject.toml rather than hardcoding in requirements-dev.txt, so that uv sync stays the single source of truth.

---

Summary

Area	Assessment
Correctness	✅ The flow-causality fix is algorithmically correct
API migration	✅ Clean 1-to-1 mapping of the graphqomb 0.3.x API
Test coverage	⚠️ Missing idempotency and multi-predecessor cases
Code clarity	⚠️ min_measure_time naming and the loop pattern could be clearer
Dependency management	⚠️ ruff pin should be intentional and justified

Happy to approve once the test gaps and the ruff pin rationale are addressed (or acknowledged as intentional).

[Copilot]

Pull request overview

This pull request updates ls-pattern-compile to be compatible with graphqomb>=0.3.1 by adjusting compilation to the newer qompile() logical-observable API and the updated stim_compile() noise model interface, while also proactively fixing manual scheduler causality issues that graphqomb now validates earlier.

Changes:

• Bump graphqomb minimum version to 0.3.1 and adapt compilation to updated qompile() / stim_compile() APIs.
• Add a scheduler pre-validation adjustment step that delays flow-dependent measurements to satisfy DAG causality.
• Pin the CI Ruff version via requirements-dev.txt and add a regression test covering the causality delay behavior.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
lspattern/compiler.py	Updates compilation pipeline for graphqomb 0.3.x (scheduler validation, logical observables via qompile, noise via noise_models).
tests/test_graph_block_yaml.py	Adds a regression test ensuring equal-time flow dependencies are split by delaying measurements.
requirements.txt	Raises graphqomb dependency floor to >=0.3.1.
requirements-dev.txt	Pins ruff so CI uses a known-good version referenced by the workflow.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.