Phase 3 → Phase 4 Report
Likely design issues
L1. tovec and shiftrange are exported but are one-liner wrappers around Base
tovec(v) returns v if it is already an AbstractVector, otherwise collect(v) (effectively). shiftrange(r, s) returns r .+ s. Both appear to be helper utilities extracted
from application code and left in the public API. Neither has a domain-specific identity that a user of this package would look for here. If a user needs r .+ s they write
it; if they need collect they write it. Exporting these pollutes the namespace with no gain.
L2. mismatch_apertures is not exported but is documented and called by users
mismatch is exported. mismatch_apertures is the primary high-level entrypoint for the aperture workflow — the conceptual centerpiece of the package — yet it is not in the
export list. The asymmetry is inconsistent. A user loading this package to do aperture-based registration would expect mismatch_apertures to be exported.
L3. padsize(blocksize::Int, maxshift::Int, dim::Int) leaks FFT strategy selection
The three-argument form padsize(blocksize::Int, maxshift::Int, dim::Int) selects different rounding strategies depending on dim (power of 2 for dim 1, nextprod otherwise).
This is a FFTW implementation detail about how real-FFT plans are structured. Exposing dim as a public parameter means callers need to know this convention. If the
two-argument Dims form already encapsulates the per-dimension logic, the three-argument form looks like a private helper that was exported accidentally or for testing.
L4. set_FFTPROD / FFTPROD global mutable state
FFTPROD is a module-level mutable vector governing FFT size selection in padsize and padranges. set_FFTPROD is the setter. Global mutable state is a common source of
subtle bugs in Julia (thread-unsafe, order-dependent, hard to test in isolation). The only anticipated use case appears to be a caller who wants to use a different FFT
library. This would be better served by passing the prime-factor list as a keyword argument to padsize/padranges, or by a backend-defined constant.
Design questions
Q1. each_point dispatches on two incompatible representations — is there an intended canonical form?
each_point has two methods: one for an array-of-tuples/vectors, one for a real matrix (column-per-point). Both are accepted by allocate_mmarrays and presumably by the
aperture workflow. Was the intent that users are free to use either representation interchangeably? If so, is there a reason a single abstract "point cloud" type isn't
defined to give this a declared API contract? Or was one representation intended to be the canonical form and the other a compatibility shim?
Q2. allocate_mmarrays has a gridsize overload that internally calls aperture_grid — should these be two functions?
The third method of allocate_mmarrays accepts a gridsize tuple directly and creates the aperture grid internally. This is convenient but hides a step that the two-argument
form exposes. The question is: is the gridsize overload a convenience wrapper intended for the common case, or does it mean aperture_grid is not truly necessary for the
user to see at all?
Q3. argmin_mismatch is referenced by register_translate but doesn't appear in the export list
If argmin_mismatch extracts the best-shift CartesianIndex from a MismatchArray, a user who calls mismatch directly (rather than register_translate) would naturally want it
too. Is it intentionally non-exported, or was it overlooked? If it is useful externally, it should be exported; if not, register_translate should be the only path.
Q4. DimsLike vs WidthLike — why two aliases with overlapping coverage?
DimsLike = Union{AbstractVector{Int}, Dims} and WidthLike = Union{AbstractVector, Tuple}. WidthLike is strictly broader (any element type). Were these distinguished
because aperture widths can be floating-point while dimensions are always integers? If so, should WidthLike be Union{AbstractVector{<:Real}, NTuple{N,<:Real} where N}? The
current definition accepts, e.g., a Tuple{String}, which is almost certainly not intended.
Q5. correctbias! and truncatenoise! — is there a reason there are no non-mutating forms?
Both operate on MismatchArray in-place. For truncatenoise! especially, a user computing a threshold interactively might want to try multiple values without re-running the
backend. Were non-mutating forms considered and rejected (memory pressure in large volumetric workflows?), or not considered?
Observations
O1. issamesize, nanval, computeoverlap, FFTPROD are tested by qualified access but not exported or marked public
All four appear in the test suite via RegisterMismatchCommon.issamesize(...) etc. In Julia 1.11+ these could be marked public without being exported, signaling that they
are stable internal API. Currently their status is ambiguous.
O2. MismatchArray and NumDenom live in RegisterCore but are the central data types here
Users of this package encounter MismatchArray and NumDenom constantly, but they are owned by RegisterCore. This means the type hierarchy "lives" one package away from the
package that defines the primary operations on those types. This is probably intentional (to allow RegisterMismatch and RegisterMismatchCuda to share the type
definitions), but it means RegisterMismatchCommon's own conceptual center of gravity is borrowed rather than owned.
O3. nanpad pads the smaller array with NaN but both return arrays have the same size
nanpad returns (fixedpad, movingpad) where both share the same size. If a user passes two arrays of the same size, the function still copies both (or is a no-op that
copies). The behavior on same-size inputs is worth documenting — it is either a trivial copy or a size-check pass-through, and users may be surprised by the allocation.
O4. register_translate is a high-level convenience that fully wraps mismatch + threshold + argmin_mismatch
This is the only end-to-end function in the package. As a convenience wrapper it is reasonable, but it only handles the single-image-pair case, not the aperture-array
case. The naming (register_translate) is also slightly surprising in a "Common" package — it reads as a backend-specific function.
Overall design characterization
RegisterMismatchCommon is a clean, focused package with a well-identified purpose and a natural decomposition of the aperture workflow into composable pieces. The type
design is minimal (most types live in RegisterCore) and the function set maps directly onto the computation graph a user needs. The main tension is between a lean,
opinionated API and a collection of exported utilities — specifically, thin helpers like tovec, shiftrange, and the three-argument padsize that look like application-code
extractions rather than designed public API. The one structural concern is the mutable global FFTPROD/set_FFTPROD, which is fragile. The highest-leverage changes would be
(1) auditing and trimming the export list to remove utilities that aren't genuine public API, and (2) eliminating or encapsulating the global mutable state in FFTPROD.
Phase 5 — Values clarification
Before I write a plan, I'd like your input on a few questions raised by the review:
- Scope and audience: The package currently exports a mix of high-level aperture-workflow functions and low-level FFT-size utilities. Is the intended audience strictly
other package authors in the HolyLab stack (who might legitimately need padsize/padranges), or also domain scientists who should only ever see the high-level aperture API?
Should low-level utilities be trimmed from the export list, or are they deliberately offered for extension?
- Central abstraction: MismatchArray/NumDenom live in RegisterCore, not here. Is that intentional and permanent? Or is there an argument for moving them here, given that
this package is where all the operations on them are defined?
- Composability and Base relationship: How important is it that mismatch_apertures be exported? (It is currently not.) And should argmin_mismatch be exported for users
who call mismatch directly?
- Error and failure model: truncatenoise! and correctbias! are mutating-only. Is that a deliberate memory-pressure decision, or would non-mutating forms be welcome?
- FFTPROD global state: Is there a concrete use case driving set_FFTPROD? If it exists only for testing, would a keyword argument on padsize/padranges serve better?
Phase 3 → Phase 4 Report
Likely design issues
L1. tovec and shiftrange are exported but are one-liner wrappers around Base
tovec(v) returns v if it is already an AbstractVector, otherwise collect(v) (effectively). shiftrange(r, s) returns r .+ s. Both appear to be helper utilities extracted
from application code and left in the public API. Neither has a domain-specific identity that a user of this package would look for here. If a user needs r .+ s they write
it; if they need collect they write it. Exporting these pollutes the namespace with no gain.
L2. mismatch_apertures is not exported but is documented and called by users
mismatch is exported. mismatch_apertures is the primary high-level entrypoint for the aperture workflow — the conceptual centerpiece of the package — yet it is not in the
export list. The asymmetry is inconsistent. A user loading this package to do aperture-based registration would expect mismatch_apertures to be exported.
L3. padsize(blocksize::Int, maxshift::Int, dim::Int) leaks FFT strategy selection
The three-argument form padsize(blocksize::Int, maxshift::Int, dim::Int) selects different rounding strategies depending on dim (power of 2 for dim 1, nextprod otherwise).
This is a FFTW implementation detail about how real-FFT plans are structured. Exposing dim as a public parameter means callers need to know this convention. If the
two-argument Dims form already encapsulates the per-dimension logic, the three-argument form looks like a private helper that was exported accidentally or for testing.
L4. set_FFTPROD / FFTPROD global mutable state
FFTPROD is a module-level mutable vector governing FFT size selection in padsize and padranges. set_FFTPROD is the setter. Global mutable state is a common source of
subtle bugs in Julia (thread-unsafe, order-dependent, hard to test in isolation). The only anticipated use case appears to be a caller who wants to use a different FFT
library. This would be better served by passing the prime-factor list as a keyword argument to padsize/padranges, or by a backend-defined constant.
Design questions
Q1. each_point dispatches on two incompatible representations — is there an intended canonical form?
each_point has two methods: one for an array-of-tuples/vectors, one for a real matrix (column-per-point). Both are accepted by allocate_mmarrays and presumably by the
aperture workflow. Was the intent that users are free to use either representation interchangeably? If so, is there a reason a single abstract "point cloud" type isn't
defined to give this a declared API contract? Or was one representation intended to be the canonical form and the other a compatibility shim?
Q2. allocate_mmarrays has a gridsize overload that internally calls aperture_grid — should these be two functions?
The third method of allocate_mmarrays accepts a gridsize tuple directly and creates the aperture grid internally. This is convenient but hides a step that the two-argument
form exposes. The question is: is the gridsize overload a convenience wrapper intended for the common case, or does it mean aperture_grid is not truly necessary for the
user to see at all?
Q3. argmin_mismatch is referenced by register_translate but doesn't appear in the export list
If argmin_mismatch extracts the best-shift CartesianIndex from a MismatchArray, a user who calls mismatch directly (rather than register_translate) would naturally want it
too. Is it intentionally non-exported, or was it overlooked? If it is useful externally, it should be exported; if not, register_translate should be the only path.
Q4. DimsLike vs WidthLike — why two aliases with overlapping coverage?
DimsLike = Union{AbstractVector{Int}, Dims} and WidthLike = Union{AbstractVector, Tuple}. WidthLike is strictly broader (any element type). Were these distinguished
because aperture widths can be floating-point while dimensions are always integers? If so, should WidthLike be Union{AbstractVector{<:Real}, NTuple{N,<:Real} where N}? The
current definition accepts, e.g., a Tuple{String}, which is almost certainly not intended.
Q5. correctbias! and truncatenoise! — is there a reason there are no non-mutating forms?
Both operate on MismatchArray in-place. For truncatenoise! especially, a user computing a threshold interactively might want to try multiple values without re-running the
backend. Were non-mutating forms considered and rejected (memory pressure in large volumetric workflows?), or not considered?
Observations
O1. issamesize, nanval, computeoverlap, FFTPROD are tested by qualified access but not exported or marked public
All four appear in the test suite via RegisterMismatchCommon.issamesize(...) etc. In Julia 1.11+ these could be marked public without being exported, signaling that they
are stable internal API. Currently their status is ambiguous.
O2. MismatchArray and NumDenom live in RegisterCore but are the central data types here
Users of this package encounter MismatchArray and NumDenom constantly, but they are owned by RegisterCore. This means the type hierarchy "lives" one package away from the
package that defines the primary operations on those types. This is probably intentional (to allow RegisterMismatch and RegisterMismatchCuda to share the type
definitions), but it means RegisterMismatchCommon's own conceptual center of gravity is borrowed rather than owned.
O3. nanpad pads the smaller array with NaN but both return arrays have the same size
nanpad returns (fixedpad, movingpad) where both share the same size. If a user passes two arrays of the same size, the function still copies both (or is a no-op that
copies). The behavior on same-size inputs is worth documenting — it is either a trivial copy or a size-check pass-through, and users may be surprised by the allocation.
O4. register_translate is a high-level convenience that fully wraps mismatch + threshold + argmin_mismatch
This is the only end-to-end function in the package. As a convenience wrapper it is reasonable, but it only handles the single-image-pair case, not the aperture-array
case. The naming (register_translate) is also slightly surprising in a "Common" package — it reads as a backend-specific function.
Overall design characterization
RegisterMismatchCommon is a clean, focused package with a well-identified purpose and a natural decomposition of the aperture workflow into composable pieces. The type
design is minimal (most types live in RegisterCore) and the function set maps directly onto the computation graph a user needs. The main tension is between a lean,
opinionated API and a collection of exported utilities — specifically, thin helpers like tovec, shiftrange, and the three-argument padsize that look like application-code
extractions rather than designed public API. The one structural concern is the mutable global FFTPROD/set_FFTPROD, which is fragile. The highest-leverage changes would be
(1) auditing and trimming the export list to remove utilities that aren't genuine public API, and (2) eliminating or encapsulating the global mutable state in FFTPROD.
Phase 5 — Values clarification
Before I write a plan, I'd like your input on a few questions raised by the review:
other package authors in the HolyLab stack (who might legitimately need padsize/padranges), or also domain scientists who should only ever see the high-level aperture API?
Should low-level utilities be trimmed from the export list, or are they deliberately offered for extension?
this package is where all the operations on them are defined?
who call mismatch directly?