Upgrade LasSynth importer: Hadamard blocks, bulk measure, and pipe simplification

[masa10-f]

Summary

• Simplify pipe generation to single-layer Short Memory Blocks (ShortXMemoryBlock/ShortZMemoryBlock), eliminating the two-layer pipe structure and stacking conflict checks
• Add bulk measure blocks (MeasureBulkXBlock, MeasureBulkZBlock) with corresponding YAML definitions
• Update LAS importer to support Hadamard blocks, invert_ancilla_order, logical_observables, and improved boundary handling
• Update CNOT spec demo YAML files to reflect new block types and boundary configurations
• Add Hadamard block test suite (test_las_hadamard.py)

Test plan

• pytest tests/test_las_hadamard.py passes
• Existing tests remain passing
• CNOT spec YAMLs correctly load and compile

🤖 Generated with Claude Code

[claude]

Pull Request Review: Upgrade LasSynth importer

Summary

This PR represents a significant architectural improvement to the LasSynth importer, introducing support for Hadamard blocks, bulk measure blocks, and simplifying the pipe generation structure. The changes are well-tested and the code quality is generally high.

Code Quality & Best Practices ✅

Strengths:

• Excellent test coverage: The new test_las_hadamard.py provides comprehensive unit tests for the Hadamard detection logic with clear test names and edge cases
• Good documentation: Functions have detailed docstrings with parameter descriptions and return types
• Type hints: Proper use of type annotations throughout (e.g., list[dict[str, Any]] | None)
• Clean separation of concerns: Helper functions are well-organized and focused on single responsibilities

Minor Suggestions:

1. Consider adding integration tests for the end-to-end YAML generation with Hadamard blocks (lspattern/importer/las.py:952-964)
2. Function complexity: _build_cube_entries (las.py:816-892) and convert_lasre_to_yamls (las.py:900-1032) are quite long. Consider extracting some logic into smaller helper functions
3. Magic numbers: The sublayer values (1, 2) in _get_pipe_logical_observables (las.py:559-575) could benefit from named constants for clarity

Potential Bugs or Issues ⚠️

Critical:
None identified.

Minor:

1. Recursive functions without depth limit: _resolve_color_kp and _resolve_color_km (las.py:396-423) use recursion to follow -1 references. While unlikely in practice, deeply nested -1 chains could cause stack overflow. Consider adding a max_depth parameter.

2. Potential index out of bounds: In _get_corr_value (las.py:484-493), the bounds check is good, but it silently returns False for out-of-bounds access. Consider logging a warning for debugging purposes.

3. Empty correlation surfaces: The code assumes correlation surface arrays exist when accessed. Consider adding defensive checks in case correlation surface data is malformed.

Performance Considerations ⚡

Good:

• Efficient use of sets for cube lookups
• Single-pass processing of cubes in most cases
• Sorting only when necessary (las.py:293, 703)

Potential Optimizations:

1. Repeated correlation surface lookups: Functions like _build_toplevel_logical_observables (las.py:750-813) make multiple correlation surface lookups. Consider caching results if performance becomes an issue.

2. Dictionary lookups in loops: The invert_map.get(coord, False) pattern (las.py:866, 886) is called repeatedly. This is fine for the current use case, but consider pre-computing if cube counts grow significantly.

Security Concerns 🔒

Low Risk:

• YAML parsing with yaml.safe_dump is appropriate (las.py:1024)
• No user input is directly executed
• No external network calls or file system writes in the importer itself

Recommendations:

1. Input validation: Consider adding bounds validation for array indices in the lasre dictionary to prevent potential denial-of-service from malformed input
2. YAML bomb protection: While safe_dump is used, consider setting size limits if YAML files will be loaded later

Test Coverage ✅

Excellent:

• New test file test_las_hadamard.py provides 179 lines of comprehensive unit tests
• Tests cover edge cases like k=0, -1 resolution chains, toggling behavior
• Multiple test classes organized by functionality

Suggestions:

1. Add integration tests that verify the complete YAML output format
2. Test error handling paths (e.g., malformed correlation surfaces)
3. Consider property-based testing for complex Hadamard detection logic

Architecture & Design 🏗️

Major Improvements:

1. Pipe simplification: Removing the two-layer pipe structure (Init + Measure) in favor of single-layer ShortMemoryBlocks is a significant simplification (las.py:989-1006)
2. Bulk measure blocks: Using MeasureBulk blocks instead of adding extra cubes at z+1 is cleaner (las.py:352-363, 660-663)
3. Hadamard support: The invert_ancilla_order tracking is well-designed with proper toggle semantics (las.py:450-481)

Concerns:

1. Removed safety check: The _check_pipe_stacking_conflicts function was removed (deleted lines 717-749). Ensure this validation is no longer needed with the new single-layer pipe structure. If pipe conflicts are still possible, this could introduce bugs.

Specific Code Issues

lspattern/importer/las.py:656-667

if km_val != 0 and kp_val != 0:
    # Both K- and K+ boundaries: use ShortMemoryBlock
    block = _k_color_to_short_memory_block(kp_val)
    k_pipe_coord = (i, j, k)
elif kp_val != 0:
    # Only K+ boundary: use MeasureBulkBlock
    block = _k_color_to_meas_bulk_block(kp_val)
    k_pipe_coord = (i, j, k)

Question: What happens when km_val == 0 and kp_val == 0? The code falls through to returning "MemoryBlock" (line 636), which seems correct, but this logic path could be more explicit.

lspattern/patch_layout/blocks/measure_bulk_z_block.yml:10

The description mentions MeasureZPartialUnit but based on the context, verify this is the correct unit type vs MeasureZShiftedUnit used in ShortZMemoryBlock.

Demo YAML Changes

The demo YAML files show consistent application of the new block types:

• Hadamard blocks with invert_ancilla_order: true
• ShortMemory blocks replacing Init+Measure pipe pairs
• MeasureBulk blocks for K+ boundaries
• Proper logical_observables annotations

The changes look correct and well-structured.

Recommendations

Before Merge:

1. ✅ Verify all existing tests pass
2. ✅ Run pytest tests/test_las_hadamard.py to confirm new tests pass
3. ⚠️ Document why _check_pipe_stacking_conflicts removal is safe
4. ⚠️ Consider adding a max recursion depth to _resolve_color_kp/km

Future Improvements:

1. Add integration tests for complete YAML generation
2. Consider performance profiling if handling large lasre structures
3. Add logging for debugging correlation surface lookups
4. Document the Hadamard detection algorithm in MEMORY.md or project docs

Overall Assessment

Verdict: Approve with minor recommendations ✅

This is a high-quality PR that makes significant architectural improvements while maintaining code quality. The test coverage for the new Hadamard functionality is excellent. The main concern is ensuring the removal of pipe stacking conflict checks doesn't introduce regressions, but the new architecture appears sound.

The code demonstrates good software engineering practices with proper type hints, comprehensive tests, and clear separation of concerns. The simplification from two-layer to single-layer pipes is a welcome improvement.

---

Review generated by Claude Code