Phase 4 — Design Review Report: BlockRegistration
Likely design issues
- compose returns a named tuple instead of a deformation (RegisterDeformation)
compose(ϕ_old, ϕ_new) returns (; ϕ, gradient). This silently breaks composition chains: compose(ϕ1, compose(ϕ2, ϕ3)) fails because the inner result is a named tuple, not an
AbstractDeformation. The overloaded ∘ operator presumably has the same signature. The gradient of the composed field is useful for optimization, but bundling it into the default return
type prevents natural pipeline composition and surprises users who expect compose to return something composable.
- penalty! has five incompatible overloads under one name (RegisterPenalty / RegisterOptimize)
The same function name covers: data-only single-frame, data+regularization single-frame, data+regularization multi-frame (with λt), regularization-only, and temporal-roughness-only.
The only dispatch signal is argument count and type. A user reading penalty!(g, ϕ, mmis) vs. penalty!(g, dp, ϕ_c) is guessing from argument order which penalty they're computing. This
is a classic case where the name covers conceptually distinct operations that have been unified for syntactic convenience.
- ColonFun is explicitly an internal implementation detail but appears in the exported surface
The conceptual mapper flags it as an internal implementation detail (ColonFun()(i::Int) returns Colon() for type-stable index construction). Exporting it means users see it in
tab-completion and names(BlockRegistration). It has no documented use case outside the package itself.
- InterpolatingDeformation alias is defined differently in two packages
In RegisterDeformation it is GridDeformation{T,N,A<:AbstractInterpolation}; in RegisterHindsight it is restricted further to A<:ScaledInterpolation with InPlace boundary conditions.
When both are loaded via using BlockRegistration, the second definition shadows or conflicts with the first. A user writing a method on InterpolatingDeformation gets different behavior
depending on which package's definition is in scope.
- fit_sigmoid is public API for a purely internal optimization step
fit_sigmoid fits data to a logistic function via Ipopt. The description says it is used internally by auto_λ to select regularization strength. Users of BlockRegistration do not need
to call it, and its presence in the public API leaks the implementation strategy of auto_λ (sigmoid-based sweep) rather than its intent (automatic regularization selection).
- qbuild is explicitly described as a debugging utility but is exported
"Useful for debugging" is the description. Functions exported from a library should have documented production use cases.
Design questions
Q1. Is AbstractDeformation load-bearing?
AbstractDeformation{T,N} has exactly one concrete public subtype: GridDeformation. Is the abstract type there because users are expected to implement custom deformation types (e.g.,
B-spline fields, thin-plate splines), or was it added speculatively? If no external subtype is intended, the abstraction adds type-hierarchy overhead without providing an interface
contract.
Q2. interpolate vs extrapolate — is the naming intentional or accidental?
Both produce an InterpolatingDeformation; the difference is boundary conditions (Flat(OnCell()) vs Line extrapolation). The names suggest the distinction is whether interpolation
happens at all, but both functions interpolate — only the BC differs. Would interpolate(ϕ, Flat(...)) / interpolate(ϕ, Line(...)) communicate the distinction more clearly, or is the
two-function API intentional?
Q3. Should DeformationPenalty remain abstract?
Like AbstractDeformation, DeformationPenalty{T,N} has exactly one public concrete subtype (AffinePenalty). Are users expected to implement custom penalty terms? If yes, what interface
must they satisfy (currently: penalty!(g, dp, ϕ_c))? If no, the abstract type is speculative.
Q4. Were low-level utilities exported for ecosystem use or by accident?
vecindex, vecgradient!, centeraxes, arraysize, mismatcharrays, tformtranslate, tformeye, tformrotate, rotation2, rotation3 feel like internal helpers or thin wrappers around
CoordinateTransformations.jl / Rotations.jl. Were these exported because related HolyLab packages depend on them, or because they seemed potentially useful and were never reviewed?
Q5. RegisterHindsight.optimize! is the primary entry point of its module but is not exported — should it have a distinct name?
The non-export is deliberate (to avoid clashing with RegisterOptimize.optimize!). But this means users must write RegisterHindsight.optimize!(...) or explicitly import it. Would a
distinct name like refine! remove the conflict while making the function available unqualified?
Q6. highpass, paddedview, trimmedview — are these intended for user pipelines or only for internal preprocessing?
highpass is described as image preprocessing for the registration pipeline. paddedview / trimmedview are SubArray helpers. Do users call these directly, or are they implementation
details of PreprocessSNF?
Q7. warp (deformation) vs transform (affine) — is the name split documented as a convention?
Both apply spatial transformations to images. The split (non-rigid → warp, affine → transform) is a sensible convention, but if a user wants to apply an affine map to an image, they
must know to call transform, not warp. Is this convention documented?
Observations
- warpgrid and nodegrid return images for visual inspection of the deformation grid. They are diagnostic utilities that feel more at home in a notebook/example than in production API.
- principalaxes / principalaxes_rotation provide rigid-registration initialization. The name principalaxes is mathematically precise but does not signal its registration-specific role
to a domain user.
- auto_λ has two signatures: one taking raw images and one taking pre-computed (cs, Qs, nodes, mmis). This is good composability design. The two entry points should be clearly
distinguished in documentation (raw vs. pre-computed).
- tinterpolate performs temporal interpolation of deformations, while interpolate(ϕ) performs spatial B-spline fitting. The shared root interpolate in two very different contexts may
cause confusion.
- convert_to_fixed / convert_from_fixed are described as "imported explicitly as internal API" by consuming packages. If they are truly internal, they should not be exported. If they
are intended as semi-public, they need public annotations and documentation.
- RegisterHindsight.optimize! is not exported but is documented and is the primary entry point of its module. It, along with penalty_hindsight and related functions, should either have
public annotations or be exported (with a distinct name to avoid the naming conflict).
- PreprocessSNF is callable (functor pattern) but the callable interface is not signaled by the type name. Users who construct a PreprocessSNF may not know to call it as pp(img).
- The package being a pure re-export hub means using BlockRegistration dumps a very large number of symbols from all sub-packages into scope. There is no filtering for what is
"user-facing API" vs. "ecosystem-internal plumbing."
Overall assessment
BlockRegistration is a well-engineered, narrow-domain library whose core abstractions (GridDeformation, AffinePenalty, MismatchArray) are coherent and load-bearing. The main tension is
between being a re-export hub (where everything from every sub-package is visible) and having a curated user-facing API (where only what users actually call is exported). The current
state leans heavily toward the former, which means users see dozens of internal helpers alongside the few functions they actually need. The two highest-leverage changes are: (1) making
compose return a GridDeformation rather than a named tuple (so composition chains work naturally), and (2) auditing exports to distinguish user-facing API from ecosystem-internal
utilities and debugging helpers — likely resulting in a significant reduction in the exported surface.
Phase 5 — Values clarification
Before I write the plan, I'd like you to answer a few brief questions. Your answers will become the tiebreaker for ambiguous decisions during implementation.
- Scope and audience: Is BlockRegistration intended primarily as a convenience re-export hub for downstream HolyLab packages, or as a user-facing library that a bioimage analyst would
using BlockRegistration directly? (This determines how aggressively to trim exports.)
- Central abstraction: AbstractDeformation and DeformationPenalty each have only one public concrete subtype. Are users expected to implement custom deformation types or custom
penalty terms? Or are these abstract types speculative/legacy?
- compose return type: Was the (; ϕ, gradient) named tuple intentional (the gradient is always needed at the call sites), or was it a convenience addition that could be moved to a
keyword argument or a separate function?
- Error/failure model: ratio uses NaN as sentinel, argmin_mismatch returns a zero index, initial_deformation returns (u0, converged::Bool). Is there a preferred philosophy going
forward — or is consistency across the sub-packages even achievable given their separate development?
- Release strategy: Some of the likely issues (fixing compose, trimming exports) would be breaking. Do you want to cut a non-breaking release before the first breaking change, or
batch everything into one terminal breaking release?
Phase 4 — Design Review Report: BlockRegistration
Likely design issues
compose(ϕ_old, ϕ_new) returns (; ϕ, gradient). This silently breaks composition chains: compose(ϕ1, compose(ϕ2, ϕ3)) fails because the inner result is a named tuple, not an
AbstractDeformation. The overloaded ∘ operator presumably has the same signature. The gradient of the composed field is useful for optimization, but bundling it into the default return
type prevents natural pipeline composition and surprises users who expect compose to return something composable.
The same function name covers: data-only single-frame, data+regularization single-frame, data+regularization multi-frame (with λt), regularization-only, and temporal-roughness-only.
The only dispatch signal is argument count and type. A user reading penalty!(g, ϕ, mmis) vs. penalty!(g, dp, ϕ_c) is guessing from argument order which penalty they're computing. This
is a classic case where the name covers conceptually distinct operations that have been unified for syntactic convenience.
The conceptual mapper flags it as an internal implementation detail (ColonFun()(i::Int) returns Colon() for type-stable index construction). Exporting it means users see it in
tab-completion and names(BlockRegistration). It has no documented use case outside the package itself.
In RegisterDeformation it is GridDeformation{T,N,A<:AbstractInterpolation}; in RegisterHindsight it is restricted further to A<:ScaledInterpolation with InPlace boundary conditions.
When both are loaded via using BlockRegistration, the second definition shadows or conflicts with the first. A user writing a method on InterpolatingDeformation gets different behavior
depending on which package's definition is in scope.
fit_sigmoid fits data to a logistic function via Ipopt. The description says it is used internally by auto_λ to select regularization strength. Users of BlockRegistration do not need
to call it, and its presence in the public API leaks the implementation strategy of auto_λ (sigmoid-based sweep) rather than its intent (automatic regularization selection).
"Useful for debugging" is the description. Functions exported from a library should have documented production use cases.
Design questions
Q1. Is AbstractDeformation load-bearing?
AbstractDeformation{T,N} has exactly one concrete public subtype: GridDeformation. Is the abstract type there because users are expected to implement custom deformation types (e.g.,
B-spline fields, thin-plate splines), or was it added speculatively? If no external subtype is intended, the abstraction adds type-hierarchy overhead without providing an interface
contract.
Q2. interpolate vs extrapolate — is the naming intentional or accidental?
Both produce an InterpolatingDeformation; the difference is boundary conditions (Flat(OnCell()) vs Line extrapolation). The names suggest the distinction is whether interpolation
happens at all, but both functions interpolate — only the BC differs. Would interpolate(ϕ, Flat(...)) / interpolate(ϕ, Line(...)) communicate the distinction more clearly, or is the
two-function API intentional?
Q3. Should DeformationPenalty remain abstract?
Like AbstractDeformation, DeformationPenalty{T,N} has exactly one public concrete subtype (AffinePenalty). Are users expected to implement custom penalty terms? If yes, what interface
must they satisfy (currently: penalty!(g, dp, ϕ_c))? If no, the abstract type is speculative.
Q4. Were low-level utilities exported for ecosystem use or by accident?
vecindex, vecgradient!, centeraxes, arraysize, mismatcharrays, tformtranslate, tformeye, tformrotate, rotation2, rotation3 feel like internal helpers or thin wrappers around
CoordinateTransformations.jl / Rotations.jl. Were these exported because related HolyLab packages depend on them, or because they seemed potentially useful and were never reviewed?
Q5. RegisterHindsight.optimize! is the primary entry point of its module but is not exported — should it have a distinct name?
The non-export is deliberate (to avoid clashing with RegisterOptimize.optimize!). But this means users must write RegisterHindsight.optimize!(...) or explicitly import it. Would a
distinct name like refine! remove the conflict while making the function available unqualified?
Q6. highpass, paddedview, trimmedview — are these intended for user pipelines or only for internal preprocessing?
highpass is described as image preprocessing for the registration pipeline. paddedview / trimmedview are SubArray helpers. Do users call these directly, or are they implementation
details of PreprocessSNF?
Q7. warp (deformation) vs transform (affine) — is the name split documented as a convention?
Both apply spatial transformations to images. The split (non-rigid → warp, affine → transform) is a sensible convention, but if a user wants to apply an affine map to an image, they
must know to call transform, not warp. Is this convention documented?
Observations
to a domain user.
distinguished in documentation (raw vs. pre-computed).
cause confusion.
are intended as semi-public, they need public annotations and documentation.
public annotations or be exported (with a distinct name to avoid the naming conflict).
"user-facing API" vs. "ecosystem-internal plumbing."
Overall assessment
BlockRegistration is a well-engineered, narrow-domain library whose core abstractions (GridDeformation, AffinePenalty, MismatchArray) are coherent and load-bearing. The main tension is
between being a re-export hub (where everything from every sub-package is visible) and having a curated user-facing API (where only what users actually call is exported). The current
state leans heavily toward the former, which means users see dozens of internal helpers alongside the few functions they actually need. The two highest-leverage changes are: (1) making
compose return a GridDeformation rather than a named tuple (so composition chains work naturally), and (2) auditing exports to distinguish user-facing API from ecosystem-internal
utilities and debugging helpers — likely resulting in a significant reduction in the exported surface.
Phase 5 — Values clarification
Before I write the plan, I'd like you to answer a few brief questions. Your answers will become the tiebreaker for ambiguous decisions during implementation.
using BlockRegistration directly? (This determines how aggressively to trim exports.)
penalty terms? Or are these abstract types speculative/legacy?
keyword argument or a separate function?
forward — or is consistency across the sub-packages even achievable given their separate development?
batch everything into one terminal breaking release?