HolyLab / HolyLab/RegisterWorkerApertures.jl
Design review report
Nobody has claimed this yet.
- Dominant language
- Julia
- Stars
- 1
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
Phase 3 + 4 — Coherence Checks & Report
Likely design issues
-
myconvert appears to be dead code
The function myconvert(::Type{Array{T}}, A) is defined but, according to the conceptual map, is "not called in the visible source." It reimplements conversion logic under a different
name rather than extending Base.convert. If it is truly uncalled, it is dead code and should be removed. If it is called somewhere, it should either extend
Base.convert(::Type{Array{T}}, A) or, if the semantics differ, be given a name that makes the divergence intentional. -
load_mm_package is exported but is an implementation detail
load_mm_package(dev) dynamically loads either RegisterMismatch (CPU) or CUDA + RegisterMismatchCuda (GPU) at runtime. This is an internal bootstrapping step — users never call it
directly; init! does. Exporting it leaks the package's dynamic-loading strategy into the public API, creates a footgun (calling it twice or at the wrong time), and forces users to know
about a plumbing detail they shouldn't care about. -
Apertures mixes algorithm parameters with runtime/device state
The struct holds both immutable conceptual parameters (fixed image, aperture node grid, shift bounds, regularization strength, preprocessing function) and mutable runtime state (CUDA
device context, d_fixed CuArray buffer, d_moving CuArray buffer, CMStorage). As a result, Apertures is mutable — but the mutation is entirely for GPU state management, not for
algorithm parameter updates. This conflation means that serializing, copying, or inspecting the algorithm configuration requires awareness of opaque GPU handles.
Design questions
Q1. Are all four type parameters on Apertures{A,T,K,N} load-bearing for dispatch?
The conceptual map guesses N = dimensionality, T = element type, A = array type, K = node coordinate type. If any of these parameters is never used in a method signature to specialize
behaviour (only to hold a concrete type at construction time), it is a phantom parameter that complicates instantiation and error messages without providing dispatch benefit.
Q2. Was λrange accepting both a scalar and a (λmin, λmax) tuple intentional?
The constructor takes λrange as either a scalar λ (fixed regularization) or a (λmin, λmax) tuple (auto-selection). This is convenient but hides a meaningful distinction in a single
argument: the algorithm follows a different code path depending on the type. Should these be two separate keyword arguments (λ vs. λ_range), or even two constructor methods, so the
calling code is self-documenting about which mode is being selected?
Q3. Should monitor and monitor! live in this package or only in RegisterWorkerShell?
These utilities are described as "re-exported via RegisterWorkerShell," yet they appear in this package's exported surface area. If their implementation is in RegisterWorkerShell, this
package only re-exports them; if the implementation is here and RegisterWorkerShell re-exports from here, the ownership is inverted. Either way, the question is: do these belong to
the apertured worker specifically, or to any worker?
Q4. Is it safe to share a single Apertures across threads, or must each thread own its own?
The conceptual map notes tid (thread ID) and dev (device) fields, and mentions that monitor has a vector version for multi-threaded use. This suggests the intended pattern is one
Apertures per thread. But is that contract documented? A user who passes the same Apertures to two threads would get silent data corruption from the shared GPU buffers.
Observations
- cuda_init! is non-exported but performs a meaningful multi-step operation (allocates device memory, builds CMStorage). Advanced users debugging GPU state have no clean way to call it
directly or inspect its effects. - cudatype is a pure, domain-agnostic helper (maps a Julia element type to a CUDA-safe float type). Other packages in the BlockRegistration ecosystem likely need the same logic; it
could live in a shared utility package or in RegisterMismatchCuda rather than here. - The overall shape of the package is tight and purposeful — one concrete type, one primary operation, one ecosystem role. The design is coherent at the high level.
Overall characterization
RegisterWorkerApertures is a well-scoped plugin package with a clear single identity. Its main structural tension is between the simplicity demanded by the AbstractWorker interface
(construct once, call init!, call worker in a loop, call close!) and the complexity of GPU state management, which pushes implementation details into the public type. The two
highest-leverage changes would be: (a) making load_mm_package private or calling it internally so no user ever needs to know it exists, and (b) auditing the dead code (myconvert) and
phantom type parameters on Apertures to tighten the API surface before a v1 release.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by tracing the mentioned entry points and symbols: init!, worker, close!, myconvert, load_mm_package, Apertures, monitor, cuda_init!, and cudatype. Resolve the API, ownership, parameter, and thread-safety questions raised in the report, then document the decisions and verify that the resulting public surface and runtime behavior match them.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- julia
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100