Add liblsqecc slices-to-canvas importer#113
Conversation
PR Review: Add liblsqecc slices-to-canvas importerOverall, 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 PracticesStrengths:
Suggestions:
Potential Bugs or IssuesMinor:
Performance Considerations
Security ConcernsGood:
Suggestion: Test CoverageExcellent coverage of:
Missing test cases:
SummaryRecommendation: Approve with minor suggestions This is high-quality code ready to merge. Suggestions are minor improvements for follow-up PRs. Strong points:
Great work! |
…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>
Pull Request Review: Add liblsqecc slices-to-canvas importerSummaryThis 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: ✅ ExcellentStrengths
Minor Observations
Potential Bugs:
|
Code Review: PR #113 - LibLsQecc Slices-to-Canvas ImporterThank you for this substantial contribution! Overall code quality is good with solid engineering practices, but several areas need attention. Executive SummaryStrengths:
Concerns:
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
Security✅ No security concerns identified. Proper file handling, safe parsing, no injection risks. Test Coverage
RecommendationsMust Fix (Blocking):
Should Fix (High Priority): Positive HighlightsExceptional work on:
The architecture is sound. Issues are mainly test completeness and maintenance, not fundamental design flaws. Final Verdict✅ 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! |
PR #113 Code Review: liblsqecc slices-to-canvas importerThank 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. ✅ StrengthsCode Quality
Engineering Excellence
Security
🔴 Priority 1: Fix Before Merge1. Type Safety Issue (
|
| 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.
PR Review: Add liblsqecc slices-to-canvas importerGood work overall — the core importer logic in Blocking issues1. Hardcoded local absolute paths in example filesSeveral example scripts were committed with developer-local absolute paths that will break for anyone else:
These should be reverted to the original relative placeholder paths (e.g., 2.
|
| 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.
Summary
Changes
lspattern/importer/liblsqecc.py: New module (749 lines) — slices JSON validation, cube/pipe entry generation, ancilla component analysislspattern/importer/__init__.py: Export new public API symbolstests/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
pytest tests/test_importer_liblsqecc.py🤖 Generated with Claude Code