Design Review — RegisterWorkerAperturesMismatch
Package summary
RegisterWorkerAperturesMismatch is a single-worker plugin for the RegisterWorkerShell driver loop. It encapsulates the "apertured mismatch" computation — computing per-aperture
quadratic-fit mismatch statistics across a time series of images — and produces Es, cs, Qs, mmis arrays for downstream optimization by RegisterOptimize. The entire public API is 186
lines in one file.
Likely design issues
-
Module-level global old_active_context (src line 47)
cuda_init! and close! communicate through a bare module-level global. This is not thread-safe: if two AperturesMismatch workers with different dev values are initialized concurrently
(which is the intended multi-GPU use case), the second write to old_active_context silently clobbers the first. The intent is to restore the calling thread's prior CUDA context after
close!, but the shared global defeats that intent. This should be instance state on AperturesMismatch itself.
-
Dual-role untyped fields Es, cs, Qs, mmis (src lines 22–25)
These fields are declared without types and serve two distinct roles: before the driver initializes shared arrays they hold ArrayDecl descriptors, after initialization they hold actual
arrays. The struct never "knows" which phase it is in, the fields cannot be dispatched on, and any method that touches them is type-unstable. The design of RegisterWorkerShell's
ArrayDecl protocol may make this unavoidable, but it is worth asking whether an additional field (a flag or a separate initialized state) could make the lifecycle explicit.
-
cs name collision in worker (src line 141)
Inside the CUDA branch, cs = coords_spatial(img) reuses the name cs for spatial coordinate indices. Two lines later, outside the branch, cs = Array{SVector{N,T}}(undef, gridsize...)
declares the quadratic-coefficient output array, also named cs. The inner cs does not escape its scope (no bug), but the collision makes the function locally confusing and
maintenance-prone.
-
eval for dynamic CUDA loading (src line 32)
load_mm_package uses eval(:(using CUDA, RegisterMismatchCuda)) to load GPU support at runtime. This is a necessary workaround for conditional GPU loading, but eval at module scope runs
with module-level resolution, can silently fail if the packages are not installed, and is difficult to test. The alternative pattern (Base.require or an extension mechanism) is more
explicit.
Design questions
Q1. Should monitor and monitor! be in this package's export list?
Both are re-exported from RegisterWorkerShell. They appear here because users of this package need them, but they are not defined here. Is this package intended to be the user-facing
entry point (so re-exporting is deliberate), or should users be expected to import RegisterWorkerShell directly?
Q2. The README ends with "this approach is not currently recommended."
This is the only description of the package's status. Was this a temporary note that was never resolved? Is the package deprecated in favour of RegisterWorkerApertures? Or is the
temporal-regularization use case still valid for specific applications? The answer affects whether maintenance effort is warranted.
Q3. Should preprocess be a typed field?
The docstring says it's "likely of type PreprocessSNF, but could be a function." Leaving it untyped allows any callable, which is good for generality. The downside is that type
instability in worker propagates through algorithm.preprocess(moving0). Was this typed at some point and then relaxed, or was it always intentionally open?
Q4. correctbias is both a field and a code-path switch in worker.
The correctbias::Bool field controls whether correctbias!(mms) is called. Is there a use case for toggling this per time point (which would require it to be a field), or is it purely a
constructor-time choice? If the latter, there is no advantage to storing it as mutable state.
Q5. The CUDA path reconstructs aperture_centers on every call to worker.
aperture_grid(...) is called every time point in the CUDA branch but not in the CPU branch (where mismatch_apertures computes it internally). Was this an intentional performance
trade-off or an oversight from porting the CPU path?
Observations
- preprocess and the five untyped fields (Es, cs, Qs, mmis, preprocess) are the only sources of type instability in what is otherwise a tightly typed struct. Adding @code_warntype
output to the test suite would make regressions visible.
- init!, close!, and load_mm_package are imported from RegisterWorkerShell but not exported. They are part of the AbstractWorker protocol and will be called by the driver. Users who
want to use the worker outside the driver loop need to know about them, but they are undocumented from this package's perspective.
- cudatype and _copy_mm! are pure internals tested directly in test/internals.jl. This is good defensive practice for internal helpers.
- No show method. For a struct used at the REPL (users inspect the algorithm object), a one-line show displaying nodes, maxshift, and dev would be ergonomically useful.
- thresh == nothing (src line 117) should be thresh === nothing or isnothing(thresh) — the == form can call user-defined == on thresh, which is technically incorrect here.
Overall characterization
The package is coherent for what it is: a thin, domain-specific adapter that connects the RegisterWorkerShell infrastructure to the RegisterMismatch computation. The API surface is
minimal and the code is short. The main tension is between the package's role as a driver-framework plugin (where ArrayDecl dual-role fields and framework imports are imposed by the
protocol) and the goal of being a clean, self-contained module (where those same things look like leaking implementation accidents). The two highest-leverage changes would be: (a)
moving old_active_context from a module global into the AperturesMismatch struct (correctness + thread safety), and (b) clarifying in the README whether the package is actively
recommended, deprecated, or niche — because that answer determines whether the other findings are worth addressing.
Phase 5 — Values clarification
Before I write the plan, I have a few questions for you:
- Scope and audience: Is this package still actively used/recommended, or is the README note accurate — i.e., is it essentially in maintenance-only mode? Should fixes focus on
correctness/safety only, or is a broader API cleanup worthwhile?
- Central abstraction: The ArrayDecl dual-role pattern (fields as "declarations before init, arrays after") is imposed by RegisterWorkerShell. Is changing that protocol in scope, or
should this review treat it as a fixed external constraint?
- Error model / eval-based CUDA loading: Is the eval(:(using ...)) pattern something you want to replace (e.g., with a Julia package extension under ext/), or is compatibility with
older Julia versions that predate extensions a constraint?
- Breaking changes: Are breaking changes acceptable for this package, or should all fixes be non-breaking?
Design Review — RegisterWorkerAperturesMismatch
Package summary
RegisterWorkerAperturesMismatch is a single-worker plugin for the RegisterWorkerShell driver loop. It encapsulates the "apertured mismatch" computation — computing per-aperture
quadratic-fit mismatch statistics across a time series of images — and produces Es, cs, Qs, mmis arrays for downstream optimization by RegisterOptimize. The entire public API is 186
lines in one file.
Likely design issues
Module-level global old_active_context (src line 47)
cuda_init! and close! communicate through a bare module-level global. This is not thread-safe: if two AperturesMismatch workers with different dev values are initialized concurrently
(which is the intended multi-GPU use case), the second write to old_active_context silently clobbers the first. The intent is to restore the calling thread's prior CUDA context after
close!, but the shared global defeats that intent. This should be instance state on AperturesMismatch itself.
Dual-role untyped fields Es, cs, Qs, mmis (src lines 22–25)
These fields are declared without types and serve two distinct roles: before the driver initializes shared arrays they hold ArrayDecl descriptors, after initialization they hold actual
arrays. The struct never "knows" which phase it is in, the fields cannot be dispatched on, and any method that touches them is type-unstable. The design of RegisterWorkerShell's
ArrayDecl protocol may make this unavoidable, but it is worth asking whether an additional field (a flag or a separate initialized state) could make the lifecycle explicit.
cs name collision in worker (src line 141)
Inside the CUDA branch, cs = coords_spatial(img) reuses the name cs for spatial coordinate indices. Two lines later, outside the branch, cs = Array{SVector{N,T}}(undef, gridsize...)
declares the quadratic-coefficient output array, also named cs. The inner cs does not escape its scope (no bug), but the collision makes the function locally confusing and
maintenance-prone.
eval for dynamic CUDA loading (src line 32)
load_mm_package uses eval(:(using CUDA, RegisterMismatchCuda)) to load GPU support at runtime. This is a necessary workaround for conditional GPU loading, but eval at module scope runs
with module-level resolution, can silently fail if the packages are not installed, and is difficult to test. The alternative pattern (Base.require or an extension mechanism) is more
explicit.
Design questions
Q1. Should monitor and monitor! be in this package's export list?
Both are re-exported from RegisterWorkerShell. They appear here because users of this package need them, but they are not defined here. Is this package intended to be the user-facing
entry point (so re-exporting is deliberate), or should users be expected to import RegisterWorkerShell directly?
Q2. The README ends with "this approach is not currently recommended."
This is the only description of the package's status. Was this a temporary note that was never resolved? Is the package deprecated in favour of RegisterWorkerApertures? Or is the
temporal-regularization use case still valid for specific applications? The answer affects whether maintenance effort is warranted.
Q3. Should preprocess be a typed field?
The docstring says it's "likely of type PreprocessSNF, but could be a function." Leaving it untyped allows any callable, which is good for generality. The downside is that type
instability in worker propagates through algorithm.preprocess(moving0). Was this typed at some point and then relaxed, or was it always intentionally open?
Q4. correctbias is both a field and a code-path switch in worker.
The correctbias::Bool field controls whether correctbias!(mms) is called. Is there a use case for toggling this per time point (which would require it to be a field), or is it purely a
constructor-time choice? If the latter, there is no advantage to storing it as mutable state.
Q5. The CUDA path reconstructs aperture_centers on every call to worker.
aperture_grid(...) is called every time point in the CUDA branch but not in the CPU branch (where mismatch_apertures computes it internally). Was this an intentional performance
trade-off or an oversight from porting the CPU path?
Observations
output to the test suite would make regressions visible.
want to use the worker outside the driver loop need to know about them, but they are undocumented from this package's perspective.
Overall characterization
The package is coherent for what it is: a thin, domain-specific adapter that connects the RegisterWorkerShell infrastructure to the RegisterMismatch computation. The API surface is
minimal and the code is short. The main tension is between the package's role as a driver-framework plugin (where ArrayDecl dual-role fields and framework imports are imposed by the
protocol) and the goal of being a clean, self-contained module (where those same things look like leaking implementation accidents). The two highest-leverage changes would be: (a)
moving old_active_context from a module global into the AperturesMismatch struct (correctness + thread safety), and (b) clarifying in the README whether the package is actively
recommended, deprecated, or niche — because that answer determines whether the other findings are worth addressing.
Phase 5 — Values clarification
Before I write the plan, I have a few questions for you:
correctness/safety only, or is a broader API cleanup worthwhile?
should this review treat it as a fixed external constraint?
older Julia versions that predate extensions a constraint?