diff --git a/ASSESSMENT.md b/ASSESSMENT.md new file mode 100644 index 0000000..bd1ecd9 --- /dev/null +++ b/ASSESSMENT.md @@ -0,0 +1,307 @@ +# SpinStep — Assessment Report + +> **Auditor:** Copilot Agent +> **Date:** 2026-03-24 +> **Branch:** `copilot/assess-spinstep-repo-status` +> **Repo version:** 0.1.0 (Alpha) + +--- + +## Table of Contents + +1. [Executive Summary](#1-executive-summary) +2. [Current Status](#2-current-status) +3. [Shipping & Release Readiness](#3-shipping--release-readiness) +4. [Architecture Assessment](#4-architecture-assessment) +5. [Code Quality Scorecard](#5-code-quality-scorecard) +6. [Detailed Findings](#6-detailed-findings) +7. [Risk Register](#7-risk-register) + +--- + +## 1. Executive Summary + +SpinStep is a well-structured alpha-stage Python library implementing +quaternion-driven tree traversal. The core library is **functional, well-typed, +and has no import-time side effects**. It ships 4 public classes, 11 utility +functions, and ~725 lines of production code. + +**Overall Grade: B+** + +| Category | Grade | Summary | +|----------|-------|---------| +| Functionality | A | Core features work correctly; 46/51 tests pass | +| Code Quality | A- | 100% type hints, 100% docstrings, clean lazy loading | +| Packaging | B+ | Modern pyproject.toml; missing py.typed marker | +| Testing | B+ | Good coverage; no healpy test, no coverage metrics | +| CI/CD | A- | Active workflow on Python 3.9-3.12; no mypy/coverage steps | +| Documentation | A | 15+ markdown docs, API reference, tutorials | +| Release Readiness | B- | Functional but missing several release-gate items | + +--- + +## 2. Current Status + +### 2.1 Installation & Import + +| Check | Result | +|-------|--------| +| `pip install .` | ✅ Succeeds | +| `pip install -e ".[dev]"` | ✅ Succeeds | +| `python -m build --sdist` | ✅ Builds `spinstep-0.1.0.tar.gz` | +| `import spinstep` | ✅ Works, no side effects | +| `spinstep.__version__` | `"0.1.0"` | + +### 2.2 Test Suite + +| Metric | Value | +|--------|-------| +| Framework | pytest | +| Total tests | 51 | +| Passed | 46 | +| Skipped | 5 (4 CUDA/CuPy, 1 healpy) | +| Failed | 0 | +| Test files | 4 (`test_spinstep.py`, `test_traversal.py`, `test_discrete_traversal.py`, `test_utils.py`) | + +**What's tested:** +- ✅ `Node` — initialization, normalization, shape validation +- ✅ `QuaternionDepthIterator` — single/multi-child, thresholds, depth-first, edge cases +- ✅ `DiscreteOrientationSet` — init, factories (cube, icosahedron, sphere_grid, custom), queries +- ✅ `DiscreteQuaternionIterator` — creation, traversal, max_depth, visited tracking +- ✅ All utility functions in `quaternion_utils`, `quaternion_math`, `array_backend` + +**What's not tested:** +- ❌ `get_unique_relative_spins()` (requires healpy; test exists but always skips in CI) +- ❌ No code coverage measurement configured + +### 2.3 Lint & Type Checking + +| Tool | Result | +|------|--------| +| `ruff check spinstep/` | ✅ All checks passed | +| `mypy --strict spinstep/` | ❌ 623 errors (expected — see §6.3) | + +### 2.4 CI/CD + +| Aspect | Status | +|--------|--------| +| Workflow location | `.github/workflows/ci.yml` ✅ (active) | +| Python matrix | 3.9, 3.10, 3.11, 3.12 ✅ | +| Lint step | `ruff check spinstep/` ✅ | +| Test step | `pytest tests/` ✅ | +| mypy step | ❌ Missing | +| Coverage step | ❌ Missing | +| Build/wheel step | ❌ Missing | + +### 2.5 Public API + +The library exports 4 classes via `spinstep/__init__.py`: + +| Class | Module | Purpose | +|-------|--------|---------| +| `Node` | `node.py` | Tree node with quaternion orientation `[x,y,z,w]` | +| `QuaternionDepthIterator` | `traversal.py` | Continuous rotation-step depth-first traversal | +| `DiscreteOrientationSet` | `discrete.py` | Queryable set of discrete quaternion orientations | +| `DiscreteQuaternionIterator` | `discrete_iterator.py` | Discrete rotation-step depth-first traversal | + +**Internal utilities** (not exposed at package level — correct design): +- `utils/array_backend.py` — NumPy/CuPy backend selection +- `utils/quaternion_math.py` — batch quaternion angle computation +- `utils/quaternion_utils.py` — 9 quaternion helper functions + +--- + +## 3. Shipping & Release Readiness + +### 3.1 Release Checklist + +| # | Gate | Status | Blocker? | Notes | +|---|------|--------|----------|-------| +| 1 | Package installs cleanly | ✅ Pass | — | `pip install .` works | +| 2 | All tests pass | ✅ Pass | — | 46/46 non-optional pass | +| 3 | Linting clean | ✅ Pass | — | `ruff check` passes | +| 4 | Type checking clean | ❌ Fail | No | 623 mypy errors (strict mode); mostly stub issues | +| 5 | CI pipeline active | ✅ Pass | — | GitHub Actions on 4 Python versions | +| 6 | py.typed marker | ❌ Missing | No | PEP 561 compliance gap | +| 7 | CHANGELOG up to date | ⚠️ Partial | No | Unreleased section exists but version not bumped | +| 8 | License file present | ✅ Pass | — | MIT license | +| 9 | README accurate | ✅ Pass | — | Installation, usage, requirements correct | +| 10 | API documented | ✅ Pass | — | `docs/09-api-reference.md` exists | +| 11 | No security issues | ✅ Pass | — | No known vulnerabilities | +| 12 | Coverage measured | ❌ Missing | No | No pytest-cov or coverage.py configured | +| 13 | Build reproducible | ✅ Pass | — | `python -m build --sdist` succeeds | +| 14 | Non-core excluded from dist | ✅ Pass | — | MANIFEST.in + setuptools exclude config | + +### 3.2 Release Readiness Verdict + +**Verdict: CONDITIONALLY READY for alpha/beta release (0.1.x)** + +The library is functional and well-structured for an alpha release. There are +**no blockers** for a 0.1.x PyPI release, but the following should be addressed +before a 1.0 stable release: + +**Must-fix for 0.1.x release:** +- None (all critical items pass) + +**Should-fix before 0.2.0:** +1. Add `py.typed` marker for PEP 561 compliance +2. Add `__all__` to utility modules +3. Reduce mypy errors (at least address non-stub issues) +4. Add coverage measurement to CI + +**Should-fix before 1.0.0:** +1. Full mypy strict compliance (or document acceptable exceptions) +2. Custom exception hierarchy +3. Make internal state private (prefix with `_`) +4. Consider Protocol types for array backend abstraction + +--- + +## 4. Architecture Assessment + +### 4.1 Module Dependency Graph + +``` +spinstep/__init__.py +├── node.py (numpy only — leaf dependency) +├── traversal.py (→ node.py, numpy, scipy) +├── discrete.py (→ utils/array_backend.py, numpy, scipy, sklearn[lazy]) +└── discrete_iterator.py (→ node.py, discrete.py, numpy, scipy) + +spinstep/utils/ +├── __init__.py (empty — correct) +├── array_backend.py (cupy[lazy] → numpy fallback) +├── quaternion_math.py (no imports — pure math) +└── quaternion_utils.py (numpy, scipy, healpy[lazy]) +``` + +**Assessment:** +- ✅ **No circular dependencies** — clean DAG structure +- ✅ **Low coupling** — each module has 0-2 internal dependencies +- ✅ **Lazy loading** — cupy, sklearn.BallTree, healpy loaded only when needed +- ✅ **Clear separation** — core classes vs utility functions vs backend abstraction + +### 4.2 Module Purpose Map + +| Module | Domain | Responsibility | +|--------|--------|----------------| +| `node.py` | Core | Data model — tree node with quaternion | +| `traversal.py` | Continuous traversal | Depth-first iterator using rotation steps | +| `discrete.py` | Discrete traversal | Orientation set with spatial queries | +| `discrete_iterator.py` | Discrete traversal | Depth-first iterator over discrete orientations | +| `utils/array_backend.py` | Backend | NumPy/CuPy selection | +| `utils/quaternion_math.py` | Math | Batch angular distance computation | +| `utils/quaternion_utils.py` | Math | Quaternion manipulation helpers | + +### 4.3 Opportunities for Extending Functionality + +| Area | Description | Complexity | +|------|-------------|------------| +| Breadth-first iterator | Add `QuaternionBreadthIterator` for level-order traversal | Low | +| Serialization | Node tree to/from JSON, pickle, or HDF5 | Medium | +| Visualization | Integration with matplotlib or plotly for 3D orientation viz | Medium | +| Additional orientation sets | Fibonacci sphere, HEALPix grid, random sampling | Low | +| Interpolation | SLERP-based smooth path between orientations | Low | +| Async iteration | `__aiter__`/`__anext__` for large trees | Medium | + +### 4.4 Non-Core Directory Classification + +| Directory | Role | Should remain in repo? | Include in distribution? | +|-----------|------|:----------------------:|:------------------------:| +| `benchmark/` | Performance testing & QGNN experiments | ✅ Yes | ❌ No (correctly excluded) | +| `demos/` | Educational scripts showing library usage | ✅ Yes | ❌ No (correctly excluded) | +| `examples/` | Real-world usage patterns (GPU matching) | ✅ Yes | ❌ No (correctly excluded) | +| `docs/` | User & developer documentation | ✅ Yes | ❌ No (correctly excluded) | +| `tests/` | Unit & integration tests | ✅ Yes | ❌ No (correctly excluded) | + +--- + +## 5. Code Quality Scorecard + +| Dimension | Score | Evidence | +|-----------|-------|----------| +| **API Design** | A+ | Minimal 4-class API, clean `__all__`, good naming | +| **Type Hints** | A- | 100% coverage, but `Any` used in 5 places where more specific types are possible | +| **Docstrings** | A | Module + class + function level docstrings throughout | +| **Import Discipline** | A+ | Zero side effects, all optional deps lazy-loaded | +| **Error Handling** | B+ | Clear `ValueError`/`ImportError` messages; no custom exceptions | +| **Naming Consistency** | A- | Minor: `angle` vs `angle_threshold` parameter inconsistency | +| **Encapsulation** | B+ | Some internal state (`xp`, `rotvecs`, `stack`) not prefixed with `_` | +| **Test Quality** | B+ | 51 tests, good variety; missing coverage metrics | +| **Packaging** | B+ | Modern pyproject.toml; missing `py.typed` | +| **Documentation** | A | 15+ docs, tutorials, API ref, contributing guide | + +--- + +## 6. Detailed Findings + +### 6.1 Type Hint Issues + +The following locations use `Any` where more specific types are possible: + +| Location | Current Type | Recommended Type | Priority | +|----------|-------------|-----------------|----------| +| `quaternion_math.batch_quaternion_angle(qs1, qs2, xp)` | `Any, Any, Any → Any` | `np.ndarray, np.ndarray, ModuleType → np.ndarray` | High | +| `DiscreteOrientationSet.xp` | `Any` | `ModuleType` | Medium | +| `DiscreteOrientationSet._balltree` | `Any` | `Optional[BallTree]` | Low | +| `DiscreteOrientationSet.orientations` | `Any` | `np.ndarray` | Medium | + +### 6.2 mypy --strict Errors (623 total) + +**Breakdown by category:** + +| Category | Count | Fixable? | Effort | +|----------|-------|----------|--------| +| Missing library stubs (`scipy`, `sklearn`) | ~580 | Yes (install stubs or ignore) | Low | +| `no-any-return` (returning from untyped libs) | ~20 | Yes (type: ignore or cast) | Low | +| `attr-defined` in `get_relative_spin` | 2 | Yes (use Protocol or type param) | Medium | +| Unused `type: ignore` comments | 2 | Yes (fix or remove) | Low | +| `import-not-found` (healpy, cupy) | 2 | N/A (optional deps) | Low | + +**Recommendation:** Add `mypy` overrides in `pyproject.toml` to ignore missing stubs +for optional/third-party dependencies, and fix the ~25 actual code-level issues. + +### 6.3 Missing `__all__` in Utility Modules + +| Module | Has `__all__`? | Should have? | +|--------|:-:|:-:| +| `spinstep/__init__.py` | ✅ | — | +| `spinstep/utils/__init__.py` | ❌ | ✅ (empty list) | +| `spinstep/utils/quaternion_utils.py` | ❌ | ✅ (9 functions) | +| `spinstep/utils/quaternion_math.py` | ❌ | ✅ (1 function) | +| `spinstep/utils/array_backend.py` | ❌ | ✅ (1 function) | + +### 6.4 Internal State Exposure + +These attributes should be prefixed with `_` to signal they're not part of the +public contract: + +| Class | Attribute | Reason | +|-------|-----------|--------| +| `DiscreteOrientationSet` | `xp` | Implementation detail (backend module) | +| `DiscreteOrientationSet` | `rotvecs` | Cached internal representation | +| `DiscreteQuaternionIterator` | `stack` | Internal traversal state | + +### 6.5 Packaging Gaps + +| Gap | Impact | Fix | +|-----|--------|-----| +| No `py.typed` marker | Downstream mypy users get no type info | Create empty `spinstep/py.typed`, add to MANIFEST.in | +| `dev-requirements.txt` duplicates `[dev]` extras | Confusion about canonical source | Consider removing file or adding comment | +| No `[project.urls]` for docs/changelog | Less discoverable on PyPI | Add URLs section | + +--- + +## 7. Risk Register + +| # | Risk | Likelihood | Impact | Mitigation | +|---|------|-----------|--------|------------| +| 1 | Breaking API changes before 1.0 | High | Medium | Document API stability policy; semver 0.x allows breaking changes | +| 2 | scipy/sklearn API changes | Low | Medium | Pin minimum versions (already done); test on new releases | +| 3 | CuPy CUDA version mismatch | Medium | Low | Document supported CUDA versions; test matrix includes GPU path | +| 4 | Type hint rot | Medium | Low | Add mypy to CI pipeline; address stub issues incrementally | +| 5 | Undiscovered edge cases in traversal | Low | Medium | Expand test suite; add property-based tests | + +--- + +*End of assessment.* diff --git a/repository-refactor-master.instructions.mdTIONS.md b/repository-refactor-master.instructions.mdTIONS.md new file mode 100644 index 0000000..e2a7a2e --- /dev/null +++ b/repository-refactor-master.instructions.mdTIONS.md @@ -0,0 +1,328 @@ +# Repository Master Refactorer Instructions + +> **Purpose:** Step-by-step instructions for refactoring SpinStep into a clean, +> production-quality Python library. +> **Constraint:** Preserve all existing functionality and tests. +> **Date:** 2026-03-24 + +--- + +## Execution Rules (CRITICAL) + +* Operate in **proposal mode by default** +* **Do NOT apply changes unless explicitly instructed** +* Always present: + + 1. Current State + 2. Issues + 3. Proposed Changes +* Only move to execution after approval + +--- + +## Guiding Principles + +1. **Preserve existing functionality** — all existing tests must pass +2. **Do not weaken or remove tests** to make them pass +3. **Do not over-engineer** — avoid unnecessary abstractions +4. **Prefer clarity over cleverness** — readable code wins +5. **Keep the API small** — the 4-class public API is correct as-is +6. **Incremental changes** — each stage must be independently verifiable +7. **Do not expand scope** beyond what is explicitly defined in each stage + +--- + +## Stage Order (MANDATORY) + +Execute strictly in this order: + +1. Stage 1 — Packaging +2. Stage 2 — Public API (`__all__`) +3. Stage 3 — Internal State (private attributes) +4. Stage 4 — Type Hints +5. Stage 5 — Naming Consistency +6. Stage 6 — Exceptions +7. Stage 7 — CI/CD + +Do not skip or reorder stages. + +--- + +## Stage 1: Packaging & PEP 561 Compliance + +### 1.1 Add `py.typed` Marker + +Create: + +```bash +touch spinstep/py.typed +``` + +Update `MANIFEST.in`: + +``` +include spinstep/py.typed +``` + +--- + +### 1.2 Verify `pyproject.toml` (PEP 621) + +Verify (do not assume correctness): + +* `[build-system]` uses `setuptools>=61.0` +* `[project]` metadata is complete +* `requires-python = ">=3.9"` +* Dependencies properly defined +* Optional dependencies exist (`gpu`, `healpy`, `dev`) +* Package discovery excludes non-core directories + +If any item is incorrect → fix it. + +--- + +### 1.3 Dev Dependencies + +Keep `dev-requirements.txt`, but add header: + +``` +# Dev dependencies — canonical source is pyproject.toml +# Convenience only: pip install -r dev-requirements.txt +``` + +--- + +### 1.4 Verify Distribution Contents + +```bash +python -m build --sdist +tar tzf dist/*.tar.gz | head -30 +``` + +Confirm exclusion of: + +* benchmark/ +* demos/ +* examples/ +* tests/ +* docs/ + +--- + +### Verification + +```bash +pip install -e ".[dev]" +python -c "import spinstep; print(spinstep.__version__)" +pytest tests/ -v +``` + +--- + +## Stage 2: Define Public API (`__all__`) + +Add explicit exports to all modules. + +### Core modules + +```python +__all__ = ["Node"] +__all__ = ["QuaternionDepthIterator"] +__all__ = ["DiscreteOrientationSet"] +__all__ = ["DiscreteQuaternionIterator"] +``` + +### Utils + +```python +__all__: list[str] = [] +__all__ = ["get_array_module"] +__all__ = ["batch_quaternion_angle"] +``` + +(Keep utility exports explicit and minimal) + +--- + +### Verification + +```bash +pytest tests/ -v +``` + +--- + +## Stage 3: Internal State → Private + +Rename: + +* `xp` → `_xp` +* `rotvecs` → `_rotvecs` +* `stack` → `_stack` + +Update all internal references. + +--- + +### Test Safety Rule + +* Do NOT rewrite tests unnecessarily +* Prefer testing via public API +* Only update tests if they rely on internal attributes + +--- + +### Verification + +```bash +pytest tests/ -v +``` + +--- + +## Stage 4: Type Hints + +### Replace `Any` with concrete types + +Use: + +```python +from types import ModuleType +``` + +Introduce `Protocol` where needed. + +--- + +### Mypy Config + +Add: + +```toml +[tool.mypy] +strict = true + +[[tool.mypy.overrides]] +module = ["scipy.*", "sklearn.*", "healpy.*", "cupy.*"] +ignore_missing_imports = true +``` + +--- + +### Constraint + +Do NOT rewrite large portions of code just to satisfy typing. + +--- + +### Verification + +```bash +mypy spinstep/ +pytest tests/ -v +``` + +--- + +## Stage 5: Naming Consistency + +Rename: + +```python +angle → angle_threshold +``` + +--- + +### Scope Control + +* Only rename the parameter +* Do NOT redesign the API + +--- + +### Verification + +```bash +pytest tests/ -v +``` + +--- + +## Stage 6: Exceptions (Optional) + +Create `exceptions.py` with: + +* `SpinStepError` +* `InvalidQuaternionError` +* `BackendError` + +Ensure compatibility: + +```python +class InvalidQuaternionError(SpinStepError, ValueError): +``` + +--- + +### Constraint + +Do NOT change exception behavior beyond type replacement. + +--- + +### Verification + +```bash +pytest tests/ -v +``` + +--- + +## Stage 7: CI/CD (Only after validation) + +### Rule + +Do NOT modify CI until: + +* Tests pass +* Mypy errors are minimal + +--- + +### Add steps + +* mypy +* pytest coverage + +--- + +## Output Requirements (EVERY STEP) + +Always return: + +### User Summary + +* Installable: yes/no +* Importable: yes/no +* Tests: X passed / Y failed +* Status: short sentence + +### Technical Sections + +1. Current State +2. Issues +3. Proposed Changes +4. (Optional) Applied Changes + +--- + +## Final Constraint + +* Do NOT re-evaluate previous stages unless explicitly instructed +* Do NOT introduce new refactoring stages +* Do NOT escalate scope + +--- + +*End of instructions.*