Skip to content

Add liblsqecc slices-to-canvas importer#113

Draft
masa10-f wants to merge 22 commits into
mainfrom
liblsqecc-importer
Draft

Add liblsqecc slices-to-canvas importer#113
masa10-f wants to merge 22 commits into
mainfrom
liblsqecc-importer

Conversation

@masa10-f
Copy link
Copy Markdown
Contributor

@masa10-f masa10-f commented Feb 8, 2026

Summary

  • Add importer that converts liblsqecc slices JSON output into lspattern canvas YAML format
  • Support qubit cell cube conversion (InitZeroBlock / MemoryBlock / MeasureZBlock) with boundary string generation
  • Support ancilla connected-component analysis for pipe generation, automatic ZZ/XX basis detection, and short/long-lived ancilla block assignment

Changes

  • lspattern/importer/liblsqecc.py: New module (749 lines) — slices JSON validation, cube/pipe entry generation, ancilla component analysis
  • lspattern/importer/__init__.py: Export new public API symbols
  • tests/test_importer_liblsqecc.py: Unit tests (292 lines) — cube generation, measurement detection, init detection, boundary conversion, ancilla pipe generation, file I/O, etc.

Test plan

  • Verify all tests pass with pytest tests/test_importer_liblsqecc.py
  • Confirm no regressions in existing tests
  • Validate with actual liblsqecc output JSON

🤖 Generated with Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Feb 8, 2026

PR Review: Add liblsqecc slices-to-canvas importer

Overall, this is a well-implemented PR that adds a new importer for converting liblsqecc slices JSON into lspattern canvas YAML format. The code is well-structured, thoroughly tested, and follows good practices.

Code Quality and Best Practices

Strengths:

  • Excellent code organization with clear separation of concerns
  • Good use of type hints throughout
  • Custom LibLsQeccImportError exception with informative context
  • Well-defined module-level constants
  • Good use of frozen dataclasses for immutable structures

Suggestions:

  1. Line 45-46: Add comment explaining that solidstiched/dashedstiched handle typos in upstream liblsqecc output
  2. Line 197: Consider extracting row/column validation to reduce complexity

Potential Bugs or Issues

Minor:

  1. Lines 312-314, 376-378: Consider using assert instead of if checks for truly internal errors
  2. Line 394: No test case for distillation-side routing artifacts mentioned in comment
  3. Line 515: Error message could be clearer about 4-neighbor adjacency

Performance Considerations

  • Efficient data structures (sets, dicts, frozensets)
  • Optimal connected component analysis O(V + E)
  • Sorting may be expensive for very large canvases but acceptable

Security Concerns

Good:

  • Proper input validation
  • Safe YAML generation
  • Good file I/O error handling

Suggestion:
Consider adding file size limits to prevent memory exhaustion on large files

Test Coverage

Excellent coverage of:

  • Basic cube generation, measurement/init detection
  • Boundary edge conversion, ancilla (ZZ/XX)
  • File I/O, invalid input validation

Missing test cases:

  • Empty slices edge case
  • Conflicting ancilla/pipe basis validation paths
  • More complex multi-qubit scenarios

Summary

Recommendation: Approve with minor suggestions

This is high-quality code ready to merge. Suggestions are minor improvements for follow-up PRs.

Strong points:

  • Comprehensive input validation with helpful error messages
  • Excellent test coverage of main functionality
  • Clean separation of concerns
  • Consistent with existing codebase patterns

Great work!

masa10-f and others added 3 commits February 8, 2026 23:25
…consistency

- Add detection of direct qubit-to-qubit stitched connections (lattice
  surgery merges without intermediate ancilla bus) to generate proper
  pipe entries with O boundaries
- Relax adjacent ancilla block-kind validation to allow differing block
  types within the same bus when timelines differ, using deterministic
  lexicographic ordering for pipe assignment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…c importer

Detect DistillationQubit regions via connected-component analysis and
allow users to supply a callable template that expands each factory
into cube/pipe entries. Includes a placeholder default template and
usage documentation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactor compile_canvas_to_stim by extracting compile_canvas_to_pattern
so the MBQC pattern can be obtained independently of stim compilation.

Fix init flow analysis to exclude directions toward neighbors with cubes
below (z-1) that would cause zflow cycles via temporal edges. Gracefully
skip init flow when no valid direction exists (temporal flow from below
handles these cubes) instead of raising ValueError.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Feb 8, 2026

Pull Request Review: Add liblsqecc slices-to-canvas importer

Summary

This PR adds a substantial new feature to import liblsqecc slices JSON output into lspattern canvas YAML format. The implementation is well-structured with ~1000 lines of production code and ~300 lines of tests, plus comprehensive documentation.


Code Quality: ✅ Excellent

Strengths

  • Well-organized architecture: Clear separation of concerns with helper functions for validation, conversion, and component analysis
  • Type safety: Good use of type hints, dataclasses with frozen=True for immutability, and proper type aliases (Coord2D, Coord3D, DistillationTemplateFn)
  • Error handling: Custom LibLsQeccImportError exception with descriptive context in error messages (e.g., lspattern/importer/liblsqecc.py:160)
  • Documentation: Excellent docstrings and a dedicated 174-line user guide (docs/distillation_factory.md)
  • Code style: Consistent naming conventions, proper use of constants (_QUBIT_PATCH_TYPE, etc.)

Minor Observations

  • The module uses # noqa: C901 to suppress complexity warnings on several functions (_validate_and_normalize_slices, _detect_distillation_factories, _extract_valid_ancilla_components, _build_ancilla_blocks, convert_slices_to_canvas_yaml). While these functions handle complex logic appropriately, consider whether any could benefit from further decomposition in future refactoring.

Potential Bugs: ⚠️ Minor Issues

1. Typo in spelling variants (lspattern/importer/liblsqecc.py:22-23)

_SOLID_STITCHED_VALUES = {"solidstiched", "solidstitched"}
_DASHED_STITCHED_VALUES = {"dashedstiched", "dashedstitched"}

Issue: Includes both "stiched" (misspelled) and "stitched" (correct). This appears intentional to handle liblsqecc output inconsistencies, but should be documented if this is defensive programming against upstream typos.

2. Connected-component analysis uses DFS with recursion potential

The _detect_distillation_factories function (lspattern/importer/liblsqecc.py:323-342) uses an iterative stack-based DFS, which is good. However, for very large connected components, the stack could grow large. This is unlikely to be an issue in practice given typical distillation factory sizes.

3. Fallback z_period handling (lspattern/importer/liblsqecc.py:403-404)

if z_period == 0:
    z_period = 1  # Fallback: treat as period 1

Consideration: Silent fallback to period 1 when magic state timing cannot be detected. While reasonable, this could mask issues in malformed input. Consider logging a warning if the codebase has a logging mechanism.


Performance Considerations: ✅ Good

Efficient Algorithms

  • Connected-component analysis: O(N) where N is number of cells (appropriate)
  • Validation: Single-pass with early validation
  • Sorting uses appropriate sort keys for deterministic output

Potential Optimizations (Non-blocking)

  • _validate_and_normalize_slices validates dimensions incrementally, which is efficient
  • Multiple iterations over normalized slices (qubit extraction, ancilla extraction, distillation detection) could theoretically be combined, but current approach prioritizes clarity over micro-optimization

Security Concerns: ✅ No Issues

  • Input validation: Comprehensive validation of JSON structure with type checking
  • Safe parsing: Uses standard library json and yaml with safe loaders
  • No injection risks: String formatting uses f-strings, not user-controlled format strings
  • Path handling: Proper use of pathlib.Path for file operations with try-except blocks
  • No unsafe operations: No shell execution, eval, or dynamic code execution

Test Coverage: ✅ Comprehensive

Coverage Analysis (tests/test_importer_liblsqecc.py)

The test suite covers:

  • ✅ Basic cube generation (lines 107-118)
  • ✅ Measurement detection (lines 121-128)
  • ✅ Init detection by ID change and appearance (lines 131-144)
  • ✅ Boundary conversion (solid/dashed/stitched → Z/X/O) (lines 147-152)
  • ✅ Cell type filtering (lines 155-161)
  • ✅ Ancilla components: short-lived (ZZ/XX basis) (lines 170-208)
  • ✅ Ancilla components: long-lived (init/memory/measure) (lines 211-244)
  • ✅ Internal ancilla pipes (lines 247-263)
  • ✅ Ancilla filtering (non-measurement regions ignored) (lines 266-270)
  • ✅ Input validation (lines 273-278)
  • ✅ File I/O (lines 281-292)

Test Quality

  • Good use of helper functions (_qubit_cell, _ancilla_cell, _cube_entry, _pipe_entry)
  • Clear test names following pattern test_<behavior>_<expected_outcome>
  • Tests are focused and independent

Missing Coverage (Suggestions for future work)

  1. Distillation template functionality: No tests for default_distillation_template or the distillation factory detection system
  2. Edge cases:
    • Empty slices input
    • Single slice (no temporal dimension)
    • Maximum bounds/overflow scenarios
    • Conflicting basis detection paths (error cases at lines 612-617, 698-703)
  3. Direct qubit-to-qubit stitching: No explicit test for the code at lines 836-868
  4. Init flow interaction: Changes to lspattern/init_flow_analysis.py and lspattern/fragment_builder.py lack specific tests

Integration & Compatibility: ✅ Good

Compiler Changes (lspattern/compiler.py)

Change: Refactored compile_canvas_to_stim into two functions:

  • compile_canvas_to_pattern: Returns pattern, graph, and node_map
  • compile_canvas_to_stim: Calls the above, then generates stim circuit

Impact: ✅ Backward compatible - compile_canvas_to_stim signature unchanged
Quality: Clean separation of concerns, properly documented with docstrings

Init Flow Analysis (lspattern/init_flow_analysis.py:1366-1381)

Change: Excludes init flow directions toward neighbors with cubes at z-1 to prevent zflow cycles

# Exclude directions where the adjacent cube has a cube below (z-1).
to_exclude = set()
for side in candidates:
    vec = _SIDE_TO_VEC[side]
    neighbor = Coord3D(cube_position.x + vec.x, cube_position.y + vec.y, cube_position.z)
    below_neighbor = Coord3D(neighbor.x, neighbor.y, neighbor.z - 1)
    if below_neighbor in cube_positions:
        to_exclude.add(side)
candidates -= to_exclude

Issue: Changes return behavior from raising ValueError to silently returning when no candidates exist

if not candidates:
-    msg = f"No candidate directions for cube {cube_position} layer{sublayer} init."
-    raise ValueError(msg)
+    return  # No valid directions; temporal flow from below handles this cube

Concern: ⚠️ This behavior change could mask configuration issues where init flow genuinely should have candidates. The comment assumes "temporal flow from below handles this cube" but doesn't validate this assumption. Consider:

  • Adding a debug log when returning early
  • Validating that a cube below actually exists when returning early

Fragment Builder (lspattern/fragment_builder.py:262-282, 307-327)

Change: Wraps init flow construction in if move_vec is None: pass else: blocks instead of raising ValueError

Consistency: ✅ Consistent with init_flow_analysis changes
Risk: ⚠️ Same concern as above - silent failure mode could hide issues


Documentation: ✅ Excellent

User Documentation (docs/distillation_factory.md)

  • Comprehensive guide with background, quick start, API reference
  • Clear examples showing both basic usage and custom templates
  • Well-structured with tables and code samples
  • Explains the distinction between active outer ring cells and inactive inner cells

Code Documentation

  • Public API functions have docstrings with Parameters and Returns sections
  • Complex algorithms have inline comments explaining logic
  • Type hints throughout aid understanding

Recommendations

High Priority

  1. Add tests for distillation factory feature: The _detect_distillation_factories and default_distillation_template functions lack test coverage
  2. Validate init flow fallback assumptions: In init_flow_analysis.py, verify that temporal flow actually handles cases where init flow candidates are empty

Medium Priority

  1. Test direct qubit-to-qubit stitching: Add explicit test case for lattice surgery merges without ancilla (lines 836-868)
  2. Consider logging for fallbacks: Add debug/warning logs when fallback behavior occurs (e.g., z_period defaulting to 1)

Low Priority (Future considerations)

  1. Simplify complex functions: Consider refactoring functions with # noqa: C901 if complexity grows
  2. Document typo tolerance: Add comment explaining why both "stiched" and "stitched" are accepted

Conclusion

Overall Assessment: ⭐⭐⭐⭐ (4/5) - High Quality

This is a well-implemented feature with solid architecture, comprehensive validation, good error handling, and extensive documentation. The code demonstrates careful attention to edge cases and maintainability. The main areas for improvement are:

  • Complete test coverage for the distillation factory feature
  • Validation of the init flow fallback behavior changes

The PR is production-ready with the understanding that the identified gaps should be addressed in follow-up work.

Recommendation: ✅ Approve with minor suggestions

Great work on this substantial feature addition!


Review generated by Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Feb 9, 2026

Code Review: PR #113 - LibLsQecc Slices-to-Canvas Importer

Thank you for this substantial contribution! Overall code quality is good with solid engineering practices, but several areas need attention.


Executive Summary

Strengths:

  • Strong type safety with comprehensive type hints
  • Excellent error handling with descriptive messages
  • Well-structured modular design
  • Good core test coverage (292 lines)
  • Comprehensive documentation

Concerns:

  • Test coverage gaps (~50% of importer untested)
  • Code duplication in export.py
  • Several edge cases not handled
  • Silent fallback behaviors

Critical Issues (Must Fix)

1. Missing Distillation Factory Tests (HIGH)

Location: tests/test_importer_liblsqecc.py

The distillation factory detection code (lspattern/importer/liblsqecc.py:302-417, ~115 lines) has zero test coverage. This includes connected-component analysis, z-period detection, and template expansion.

2. Silent Z-Period Fallback (MEDIUM)

Location: lspattern/importer/liblsqecc.py:390-404

When distillation cells lack time-to-next-magic-state annotations, z_period silently defaults to 1. This could produce incorrect output without user awareness.

3. Incomplete Error Path Testing (MEDIUM)

Error handling is well-implemented but untested. Missing tests for: invalid JSON, malformed slices, file I/O errors, conflicting pipes.


Major Issues (Should Fix)

4. Code Duplication (MEDIUM)

Location: lspattern/export.py:450-565

Functions export_canvas_to_graphqomb_studio() and canvas_to_graphqomb_studio_dict() duplicate ~115 lines. Extract common logic.

5. Potential Uninitialized Variable (MEDIUM)

Location: lspattern/importer/liblsqecc.py:204-243

In _validate_and_normalize_slices(), expected_cols might not initialize if first slice has empty grid.

6. Unsafe Dictionary Access (MEDIUM)

Location: lspattern/compiler.py:48, 66

Code accesses canvas.couts keys before checking existence. Check first to avoid KeyError.


Minor Issues

  • Performance: Repeated sorted() calls in hot paths
  • Missing visualizer test coverage for detectors function (195 lines)
  • Weak aspect_ratio validation in visualizer
  • Silent highlight failures when coords not in canvas
  • Confusing unreachable code at liblsqecc.py:357-361
  • Backtracking without timeout in init_flow_analysis.py:209-226

Security

✅ No security concerns identified. Proper file handling, safe parsing, no injection risks.


Test Coverage

Component Lines Tests Quality
liblsqecc.py 971 292 ⚠️ ~50%
init_flow_analysis.py 153 175 ✅ Good
export.py 194 83 ⚠️ Basic
visualizer.py 52 69 ⚠️ Gaps

Recommendations

Must Fix (Blocking):

  1. Add distillation factory tests
  2. Add error path tests
  3. Fix uninitialized variable issue

Should Fix (High Priority):
4. Refactor export duplication
5. Fix unsafe dict access
6. Document z_period fallback


Positive Highlights

Exceptional work on:

  • Error handling with descriptive messages
  • Type safety throughout
  • Documentation (distillation_factory.md)
  • Modular design
  • Example scripts

The architecture is sound. Issues are mainly test completeness and maintenance, not fundamental design flaws.


Final Verdict

⚠️ Approve with required changes

Solid contribution adding significant value. Once must-fix items addressed (especially tests), this will be production-ready.

Estimated effort: 3-4 hours

Happy to clarify or help with fixes!

@claude
Copy link
Copy Markdown

claude Bot commented Feb 9, 2026

PR #113 Code Review: liblsqecc slices-to-canvas importer

Thank you for this substantial contribution! I've completed a thorough review of the 4,548 additions across 28 files. Overall, this is high-quality work with excellent architecture and test coverage. Below are my findings organized by priority.


✅ Strengths

Code Quality

  • Excellent modular design with clear separation of concerns (65+ well-organized helper functions)
  • Comprehensive type hints using dataclasses, generics, and protocols
  • Robust validation with 54+ explicit error checks and descriptive context-aware messages
  • Strong test coverage with 16 focused test cases covering critical paths and edge cases

Engineering Excellence

  • Efficient data structures (frozensets, LRU cache on _load_rotation_templates() at line 681)
  • Connected component analysis for distillation factory detection
  • Proper backtracking solver for init-layer flow directions
  • Clean public API with keyword-only arguments enforcing explicitness

Security

  • No vulnerabilities found (comprehensive input validation, safe file operations)
  • Proper bounds checking on array accesses
  • Type validation on all mutable structures

🔴 Priority 1: Fix Before Merge

1. Type Safety Issue (lspattern/importer/liblsqecc.py:1265-1266)

Issue: Converting dict values to tuple without validation
Risk: Silent type mismatch if entry structure is unexpected
Fix: Add validation before conversion

2. Silent Failure (lspattern/importer/liblsqecc.py:879-882)

Issue: When rotation template placement fails bounds, process_run() silently returns
Impact: User receives no warning that rotation template wasn't applied
Fix: Add logging or return status indicator

3. Performance Issue (lspattern/init_flow_analysis.py:256)

Issue: O(n) linear search for every _find_adjacent_pipe() call
Impact: Performance degradation on large canvases
Fix: Build pipe index at initialization for O(1) lookups


🟡 Priority 2: Improve Code Quality

4. Undocumented Assumption (lspattern/importer/liblsqecc.py:443)

Issue: Fallback to z_period=1 if "Time to next magic state" not found
Risk: Assumption may not always be correct for distillation cycles
Recommendation: Add warning or make configurable

5. Missing Documentation

  • Internal helper functions lack detailed docstrings
  • Performance characteristics not documented

6. Test Coverage Gaps

Missing test cases for:

  • Empty slices edge case
  • Distillation factory with z_period=0 fallback behavior
  • Rotation template file not found scenario
  • Large canvas performance/memory tests (10k+ nodes)

🟢 Priority 3: Future Enhancements

7. Limited Customization

  • Ancilla block mappings are hardcoded (lines 917-932)
  • Suggestion: Make configurable via callback similar to distillation_template

8. Minor Optimizations

  • Sorting pipes generates intermediate list - consider generator-based approach for very large canvases

9. Error Message Verbosity

  • Error messages list all keys which could be verbose for huge canvases
  • Consider truncating long lists

📊 Component Analysis

Main Importer (lspattern/importer/liblsqecc.py)

  • ⭐⭐⭐⭐⭐ Architecture & Organization
  • ⭐⭐⭐⭐ Logic Correctness
  • ⭐⭐⭐ Documentation (internal helpers need work)

Compiler Changes (lspattern/compiler.py)

  • ⭐⭐⭐⭐ Minimal & focused refactoring
  • ✅ No backward compatibility issues
  • ✅ Enhanced error reporting

Init Flow Analysis (lspattern/init_flow_analysis.py)

  • ⭐⭐⭐⭐ Elegant backtracking solver
  • ⭐⭐⭐ Performance (linear pipe search needs optimization)
  • ⭐⭐⭐⭐ Type safety

Export & Visualization

  • ⭐⭐⭐⭐ Clean API design
  • ⭐⭐⭐⭐ Proper dependency injection
  • ⭐⭐⭐⭐ Parameter validation

Test Suite

  • ⭐⭐⭐⭐ Coverage of critical paths
  • ⭐⭐⭐⭐ Well-organized fixtures
  • ⭐⭐⭐ Edge case coverage (some gaps noted above)

🎯 Overall Assessment

Category Rating Notes
Code Quality ⭐⭐⭐⭐ Well-structured, properly typed, comprehensive validation
Test Coverage ⭐⭐⭐⭐ 16 focused tests covering major paths; minor gaps in edge cases
Documentation ⭐⭐⭐ Public APIs well-documented; internal helpers need improvement
Performance ⭐⭐⭐⭐ Good overall; one O(n) operation that should be O(1)
Security ⭐⭐⭐⭐⭐ Comprehensive input validation, no vulnerabilities found
API Design ⭐⭐⭐⭐⭐ Clean, extensible, keyword-only arguments

Risk Assessment: LOW-MEDIUM

  • Core functionality is solid and well-tested
  • Recommend addressing Priority 1 items before merge
  • Priority 2 and 3 items can be addressed in follow-up PRs

✨ Recommendation

APPROVE with minor changes: This PR demonstrates excellent engineering practices and adds significant value. The Priority 1 items are straightforward fixes that will improve robustness. The codebase is production-ready once those are addressed.

Great work on this substantial feature! The architecture is extensible, the validation is thorough, and the test coverage is comprehensive. 🎉

Use canvas-level logical_observables to build GraphQOMB logicalObservableGroup instead of merging all cube/pipe couts.

Update export tests to validate canvas-level behavior and range-filter expectations.
@claude
Copy link
Copy Markdown

claude Bot commented Feb 20, 2026

PR Review: Add liblsqecc slices-to-canvas importer

Good work overall — the core importer logic in lspattern/importer/liblsqecc.py is well-structured, the validation is thorough, and the test coverage for the new module is solid. Below are issues that should be addressed before merging.


Blocking issues

1. Hardcoded local absolute paths in example files

Several example scripts were committed with developer-local absolute paths that will break for anyone else:

  • examples/canvas_layout_visualization_demo.py:
    spec_name = "/home/masato/git-repos/pyzx-mbqc/ls-pattern-compile/examples/output/adder_n4_slices_crop_x5-10_y5-11.yml"
  • examples/convert_liblsqecc_json.py:
    input_json = Path("/home/masato/git-repos/pyzx-mbqc/FTQC-compiler-survey/mf/e2edemo/output/adder_n4_slices_crop_x5-11_y5-11.json")
  • examples/export_to_graphqomb_studio.py and examples/verification_canvas.py:
    spec_name = "/home/masato/git-repos/pyzx-mbqc/SimulatorTeamWiki/..."

These should be reverted to the original relative placeholder paths (e.g., "design/cnot.yml") or replaced with clearly marked placeholder comments.

2. _load_rotation_templates silently returns {} on missing files

@lru_cache(maxsize=1)
def _load_rotation_templates() -> dict[str, _RotationTemplate]:
    ...
    for direction, filename in _ROTATION_TEMPLATE_FILES.items():
        path = base_dir / filename
        if not path.exists():
            return {}   # Silent failure — drops ALL templates if one is missing

If a single template file is missing, the function immediately returns an empty dict and rotation detection is silently disabled. This masks misconfigured installations. Consider raising a LibLsQeccImportError instead, or at minimum emitting a warning. Additionally, the path is resolved relative to the source file at import time: if the package is installed in a different location (e.g., editable install, wheel), Path(__file__).resolve().parents[2] / "examples" is very brittle. The template files should be declared as package data resources, not looked up relative to __file__.

3. lru_cache on _load_rotation_templates with mutable side-effect semantics

The @lru_cache(maxsize=1) will return the same dict across calls in the same process. Because the returned dict is mutable, callers could mutate it and poison the cache. The dict should be returned as a read-only view or made immutable via types.MappingProxyType.


Design / correctness concerns

4. process_run is a nested closure defined inside an outer for loop

for cells, zs in candidate_zs_by_cells.items():
    ...
    def process_run(start: int, end: int, cells_in_run: frozenset[Coord2D] = cells) -> None:
        ...
    for z in zs_sorted[1:]:
        ...
        process_run(run_start, run_end)
    process_run(run_start, run_end)

The default argument trick (cells_in_run: frozenset[Coord2D] = cells) is used to capture the loop variable. While it works, it is non-idiomatic Python and surprises readers. Moving the body of process_run into a separate top-level helper function with explicit parameters would be clearer and easier to test.

5. _DISTILLATION_PATCH_TYPE is defined after the module-level constants block

All other _* constants are grouped at the top of the file, but _DISTILLATION_PATCH_TYPE = "DistillationQubit" is defined after the Coord2D/Coord3D type aliases and just before the class definitions. This breaks the grouping convention and is easily missed. Move it into the constants section.

6. Ancilla boundary string is set to "" as a sentinel then mutated

cube_entries_by_coord[coord3] = {
    ...
    "boundary": "",  # Filled after pipe analysis.
}

An empty string is a valid value in some contexts. Using a sentinel like None and asserting non-None before serialization would be safer and would surface bugs more clearly.

7. Direct qubit-to-qubit stitched pipe detection only checks Bottom and Right

for side in ("Bottom", "Right"):

This avoids double-counting but means the code is implicitly assuming that Top/Left edges will always be covered by the other cell's Bottom/Right pass. This is correct given the scan order, but it is a non-obvious assumption. A brief comment explaining the deduplication logic would prevent future maintenance mistakes.

8. convert_slices_to_canvas_yaml name validation trims whitespace but doesn't validate after trim

if not name.strip():
    msg = "Canvas name must not be empty."

The function accepts name without stripping it for downstream use, meaning a name of " " raises an error, but a name of " hello " produces YAML with leading/trailing spaces. Consider stripping or validating more strictly.


Test coverage gaps

9. No test for convert_slices_file_to_canvas_yaml with a write error

test_file_conversion_writes_yaml tests the happy path only. There is no test that verifies the OSErrorLibLsQeccImportError wrapping when the output path is unwritable.

10. No test for the description parameter

convert_slices_to_canvas_yaml accepts an optional description parameter, but none of the tests verify that it is correctly embedded in the output YAML.

11. No test for the _pipe_sort_key / _cube_sort_key error paths

Both sort-key helpers raise LibLsQeccImportError for malformed entries. These are "Internal error" paths that should have at least one test to ensure they're reachable and produce the expected exception type.


Minor issues

12. itemgetter is imported but used only once; key=itemgetter(0) could be key=lambda t: t[0]

Not a bug, but the from operator import itemgetter import exists solely for sorted(timeline, key=itemgetter(0)). Using a lambda is more idiomatic for a single-use key.

13. __init__.py exports each symbol in a separate from ... import (...) block

from lspattern.importer.liblsqecc import (
    LibLsQeccImportError as LibLsQeccImportError,
)
from lspattern.importer.liblsqecc import (
    convert_slices_file_to_canvas_yaml as convert_slices_file_to_canvas_yaml,
)
from lspattern.importer.liblsqecc import (
    convert_slices_to_canvas_yaml as convert_slices_to_canvas_yaml,
)

All three can be collapsed into a single import block, consistent with the existing las imports above them.

14. requirements.txt adds kaleido and imageio/imageio-ffmpeg as mandatory dependencies

These are only needed for lspattern/video_3d.py (MP4 export), which is itself an optional/example feature. Adding heavyweight media dependencies to the core requirements.txt will affect all users. Consider making them optional extras in pyproject.toml instead.


Summary

Priority Issue
Blocking Hardcoded /home/masato/ paths in committed example files
Blocking Silent empty-dict return from _load_rotation_templates on missing files
Blocking lru_cache result is mutable and unprotected
High process_run closure / loop-variable capture pattern
Medium Ancilla "" sentinel boundary
Medium kaleido/imageio as mandatory dependencies
Low Remaining test coverage gaps and minor style issues

The importer logic itself (_extract_ancilla_components, _build_ancilla_blocks, the basis detection) is well-reasoned and the validation pipeline is solid. Addressing the path-leakage and silent-failure issues above is the main prerequisite for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant