Major overhaul: modular two-phase QC framework - #15
Merged
Conversation
Replace monolithic Standardizer with a metric-registry engine. - Two-phase evaluation: INPUT_INTEGRITY then MORPHOLOGY_QUALITY - 23 registered metrics, each self-contained and independently testable - Versioned threshold policies (policy_v1); PolicyRange for lo/hi bounds - PreparedMorphology: array-backed graph for fast metric evaluation - RunReport: structured, JSON-serialisable output (schema_version 1.1) - Public API: Metric, Applicability, register, PolicyRange all top-level - 175-test suite covering all metrics and the two-phase engine Removes: Standardizer.py, tools.py (moved to standard_morph/_archived/)
Contributor
|
Please set a versioning label of either |
There was a problem hiding this comment.
🟡 Changes recommended
The PR-close release workflow is unsafe/broken as written (force-push + missing bumpversion/setup build config) and run_qc has a concrete PreparedMorphology + input-integrity request edge case that can crash or drop requested checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR replaces the legacy monolithic QC/standardization approach with a modular, two-phase QC framework (INPUT_INTEGRITY → MORPHOLOGY_QUALITY) centered around a metric registry, an array-backed PreparedMorphology, versioned threshold policies, and structured RunReport output.
Changes:
- Introduces a registry-driven QC engine (
run_qc) with fail-fast applicability checks, scoped blocking (BUILD vs TOPOLOGY), and JSON-serialisable reporting. - Adds a modular metric suite (integrity + morphology metrics), policy versioning (
policy_v1) includingPolicyRange, and optional CCF/image helpers (atlas loading, MIP rendering). - Adds extensive unit/integration test coverage for the new engine and metrics, while moving legacy
Standardizer/toolsunderstandard_morph/_archived/.
File summaries
| File | Description |
|---|---|
| tests/test_validation.py | Updates legacy tests to import archived Standardizer/tools modules. |
| tests/test_soma_and_soma_children.py | Updates legacy soma QC test import to archived tools. |
| tests/test_qc_topology_integrity.py | Adds topology-integrity metric unit tests and engine blocking behavior tests. |
| tests/test_qc_suites.py | Adds suite composition/registration reconciliation tests. |
| tests/test_qc_soma_centroid.py | Adds soma-centroid metric tests including resource/space compatibility. |
| tests/test_qc_scalar.py | Asserts scalar metrics expose canonical value/value_label. |
| tests/test_qc_preparation.py | Adds PreparedMorphology structural/segment/component behavior tests. |
| tests/test_qc_policy_range.py | Adds PolicyRange + per-space threshold resolution tests. |
| tests/test_qc_metrics_topology.py | Adds topology metric tests (connected components, branch degree). |
| tests/test_qc_metrics_structural.py | Adds structural metric tests (roots/soma-first/identity/duplicates). |
| tests/test_qc_metrics_geometry.py | Adds geometry metric tests (edge length, soma-child distance). |
| tests/test_qc_metrics_compartment.py | Adds compartment-origin/transition metric tests. |
| tests/test_qc_local_tortuosity.py | Adds local tortuosity metric correctness/applicability tests. |
| tests/test_qc_integrity.py | Adds integrity metrics tests + two-phase engine flow tests. |
| tests/test_qc_integration.py | Adds smoke tests over real SWCs on disk. |
| tests/test_qc_filename.py | Adds filename-format metric tests and engine plumbing tests. |
| tests/test_qc_engine.py | Adds registry/suite validation and run-level behavior tests. |
| tests/test_qc_ccf_mesh.py | Adds CCF-mesh metric tests with injected synthetic atlas. |
| tests/test_max_node_parent_distance.py | Updates legacy check import to archived tools. |
| tests/test_max_node_degree.py | Updates legacy check import to archived tools. |
| tests/test_load_swc.py | Updates legacy Standardizer import to archived module. |
| tests/test_for_orphaned_nodes.py | Updates legacy check import to archived tools. |
| tests/test_cycles.py | Updates legacy cycle check import to archived tools. |
| tests/test_axon_origination.py | Updates legacy QC imports to archived modules. |
| standard_morph/swc_io.py | Adds lenient SWC reader with Horta OFFSET support. |
| standard_morph/suites.py | Adds built-in suite definitions and resolution helpers. |
| standard_morph/registry.py | Adds metric registry and register() helper (self-registration). |
| standard_morph/preparation.py | Adds array-backed PreparedMorphology with cached topology primitives. |
| standard_morph/policies.py | Adds built-in versioned policy definitions and loader. |
| standard_morph/models/qc_run.py | Adds RunReport dataclass + JSON-ready to_dict(). |
| standard_morph/models/qc_result.py | Adds MetricResult dataclass (status/value/measurements/flags). |
| standard_morph/models/qc_policy.py | Adds Policy + PolicyRange threshold types. |
| standard_morph/models/qc_context.py | Adds QCContext + enums for space/kind/resources/policy. |
| standard_morph/models/init.py | Adds model-layer exports. |
| standard_morph/metrics/soma_first_node.py | Adds soma-first-row convention metric (topology-independent). |
| standard_morph/metrics/soma_child_distance.py | Adds soma-child distance metric. |
| standard_morph/metrics/soma_at_centroid.py | Adds image-based soma centroid offset metric + optional artifact rendering. |
| standard_morph/metrics/single_root.py | Adds single-root/soma-root validation metric. |
| standard_morph/metrics/node_identity.py | Adds node identity types validation metric. |
| standard_morph/metrics/local_tortuosity.py | Adds local tortuosity metric and vectorized window computation. |
| standard_morph/metrics/filename_format.py | Adds filename convention integrity metric. |
| standard_morph/metrics/edge_length.py | Adds edge-length metric with per-space thresholds and PolicyRange support. |
| standard_morph/metrics/duplicate_coordinates.py | Adds duplicate-coordinate metric. |
| standard_morph/metrics/connected_component.py | Adds single-connected-component metric. |
| standard_morph/metrics/compartment_transitions.py | Adds dendrite compartment transition validity metric. |
| standard_morph/metrics/ccf_mesh.py | Adds CCF mesh metrics and atlas/resource resolution behavior. |
| standard_morph/metrics/branch_degree.py | Adds branch max-degree review metric (soma excluded). |
| standard_morph/metrics/base.py | Adds metric base types (phases, blocking scopes, applicability, severity). |
| standard_morph/metrics/init.py | Adds metrics package exports and import side-effects for registration. |
| standard_morph/imaging.py | Adds optional OME-Zarr soma MIP rendering utilities (lazy deps). |
| standard_morph/exceptions.py | Adds QC-specific exception types. |
| standard_morph/engine.py | Adds the suite/metric resolution + two-phase QC execution engine. |
| standard_morph/data/init.py | Adds package for bundled data discovery via importlib.resources. |
| standard_morph/atlas.py | Adds cached atlas loading + voxel conversion + in-brain masking. |
| standard_morph/_archived/Standardizer.py | Updates legacy Standardizer to import archived tools. |
| standard_morph/_archived/init.py | Documents archived legacy modules. |
| standard_morph/init.py | Adds public top-level API exports (run_qc, models, helpers). |
| pyproject.toml | Bumps version and adds extras for CCF/testing and data packaging. |
| docs/qc_overhaul_architecture_spec.md | Adds architecture specification draft for the overhaul. |
| docs/architecture_overview.md | Adds narrative architecture overview and design rationale. |
| .gitignore | Updates ignore rules for notebooks and internal state. |
| .github/workflows/dev-pr-open.yml | Adds PR-open workflow to prompt for version label. |
| .github/workflows/dev-pr-label.yml | Adds required-label enforcement workflow. |
| .github/workflows/dev-pr-close.yml | Adds merge-close workflow to bump version and publish. |
Review details
- Files reviewed: 68/73 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+22
to
+27
| - name: Install bumpversion | ||
| run: | | ||
| pip install bumpversion | ||
| - name: Apply version bump (major) | ||
| if: contains(github.event.pull_request.labels.*.name, 'major') | ||
| run: bumpversion major |
Comment on lines
+34
to
+35
| - name: Git push version bump | ||
| run: git push origin main --follow-tags --force |
Comment on lines
+10
to
+14
| jobs: | ||
| check_labels: | ||
| permissions: write-all | ||
| name: 'Check PR Labels' | ||
| runs-on: ubuntu-latest |
Comment on lines
+8
to
+12
| jobs: | ||
| add_comment: | ||
| permissions: write-all | ||
| name: 'Add Comment' | ||
| runs-on: ubuntu-latest |
Comment on lines
+262
to
+270
| else: | ||
| # A PreparedMorphology was passed in, so buildability is implied and its | ||
| # checks are skipped. But an explicitly-requested input metric that does | ||
| # not read the raw table (e.g. filename_format) must still run -- else a | ||
| # requested metric would silently vanish from the report. Such metrics | ||
| # ignore the table argument, so passing None is safe. | ||
| integrity_results, build_blocked, topology_blocked = _run_integrity_phase( | ||
| None, extra_input_names, context, policy | ||
| ) |
Comment on lines
+1
to
+7
| """Metric registration, metadata, and lookup. | ||
|
|
||
| Metrics self-register on import (see each module in ``qc/metrics``). The engine | ||
| resolves metric names to registered instances via the module-level ``REGISTRY``. | ||
| Metrics are stateless with respect to a run -- ``evaluate`` receives all inputs -- | ||
| so a single shared instance per metric is sufficient. | ||
| """ |
There was a problem hiding this comment.
🔵 Needs a closer look
The change set is a large architectural overhaul affecting core execution, public API surface, and release automation, and it includes several issues that should be resolved before approval.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
standard_morph/metrics/axon_origination.py:119
- This success message claims the axon origin was checked as being "near the soma", but that distance-to-soma check is only performed when there is exactly one soma root. When
origin_distances_to_soma_umis None (no unique soma), the message should avoid implying the distance check ran.
standard_morph/metrics/base.py:17 - The module docstring refers to a non-existent attribute name (
blocks_morphology_phase). The implementation usesblocks_on_failure(withBlockScope) to decide whether to skip the morphology phase, so the docstring should match the actual API.
standard_morph/registry.py:5
- Docstring references the old package layout (
qc/metrics), but metrics now live understandard_morph/metrics. This is misleading for contributors trying to find implementations.
.github/workflows/dev-pr-open.yml:12 permissions: write-allgrants a very broad GITHUB_TOKEN scope for a workflow that only needs to create a PR comment. Consider narrowing to least-privilege (e.g. pull-requests write) to reduce blast radius if an action is compromised.
add_comment:
permissions: write-all
name: 'Add Comment'
runs-on: ubuntu-latest
.github/workflows/dev-pr-label.yml:14
permissions: write-allis broader than necessary here and also overrides defaults. Since this job checks out the repo and reads PR labels, it can use least-privilege permissions (contents read + pull-requests read).
check_labels:
permissions: write-all
name: 'Check PR Labels'
runs-on: ubuntu-latest
- Files reviewed: 68/73 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+9
to
+13
| merge_pr: | ||
| permissions: write-all | ||
| name: 'PR Merged' | ||
| if: github.event.pull_request.merged == true | ||
| runs-on: ubuntu-latest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace monolithic Standardizer with a metric-registry engine.
Removes: Standardizer.py, tools.py (moved to standard_morph/_archived/)