API review: modernize public interface for v1.0.0

.github/workflows/CI.yml

@@ -13,7 +13,7 @@ jobs:
       fail-fast: false
       matrix:
         version:
-          - '1.10'
+          - 'min'
           - '1'
 #          - 'nightly'
         os:
@@ -23,7 +23,7 @@ jobs:
           - x64
     steps:
       - uses: actions/checkout@v2
-      - uses: julia-actions/setup-julia@v1
+      - uses: julia-actions/setup-julia@v2
         with:
           version: ${{ matrix.version }}
           arch: ${{ matrix.arch }}
@@ -47,6 +47,7 @@ jobs:
       - uses: julia-actions/julia-buildpkg@v1
       - uses: julia-actions/julia-runtest@v1
       - uses: julia-actions/julia-processcoverage@v1
-      - uses: codecov/codecov-action@v1
+      - uses: codecov/codecov-action@v5
         with:
           file: lcov.info
+          token: ${{ secrets.CODECOV_TOKEN }}

.github/workflows/TagBot.yml

@@ -28,8 +28,5 @@ jobs:
     steps:
       - uses: JuliaRegistries/TagBot@v1
         with:
-          token: ${{ secrets.GITHUB_TOKEN }}
-          # Edit the following line to reflect the actual name of the GitHub Secret containing your private key
-          ssh: ${{ secrets.DOCUMENTER_KEY }}
-          # ssh: ${{ secrets.NAME_OF_MY_SSH_PRIVATE_KEY_SECRET }}
+          token: ${{ secrets.TAGBOT_TOKEN }}
           registry: HolyLab/HolyLabRegistry

API_REVIEW_PLAN.md

@@ -0,0 +1,190 @@
+# API Review Plan
+<!-- Auto-generated by /review-api. Edit freely, but preserve chunk IDs and status values. -->
+
+## Metadata
+- **Kind**: `api`
+- **Package**: RegisterOptimize
+- **Source review date**: 2026-05-18
+- **Current version**: 0.3.6
+
+## Stated values
+Aggressive modernization. Package is pre-1.0 (v0.3.6) so breaking changes are cheap. All Tier 1 breaking changes, all Tier 2 non-breaking improvements, and all Tier 3 internal consistency fixes are approved. No deprecation shims — clean break throughout.
+
+## Release strategy
+- **Pre-breaking-release**: `no`
+- **Inter-cluster releases**: `no` — all breaking changes batch into a single v1.0.0
+
+## Baseline
+- Tests pass on the starting commit: `yes` (200/200 passing after prep work; see CHUNK-001 notes)
+- `Test.detect_ambiguities(RegisterOptimize)` count: `0`
+- Working tree clean: `yes` (at commit `7f64c5a`)
+- Current `Project.toml` version: `0.3.6`
+
+## Decisions
+<!-- Answers to `decide` chunks land here, with the chunk ID. -->
+
+## Chunks
+
+### CHUNK-001: preflight
+- **Kind**: `preflight`
+- **Originating finding**: n/a
+- **Cluster**: none
+- **Breaking**: no
+- **Description**: Establish baseline — confirm tests pass, record `Test.detect_ambiguities` count, confirm working tree is clean.
+- **Depends on**: none
+- **Verification**: full test suite green; `Test.detect_ambiguities(RegisterOptimize)` count recorded
+- **Status**: `complete`
+- **Notes**: Working tree was dirty at session start. Three prep commits landed before baseline could be recorded:
+  1. `fea6bbd` — CI/TagBot modernization (julia-actions v2, codecov v5, TagBot to HolyLabRegistry).
+  2. `ed25eee` — `Project.toml` compat bumps for `CachedInterpolations`, `CenterIndexedArrays`, and the `Register*` family to `1`.
+  3. `7f64c5a` — Source/test fixes required by the v1 deps: `TimeHessian` `*`/`mul!` now call `penalty!(y, ϕs, P.λt)` (arg order swapped per RegisterPenalty v1); the temporal-penalty test now calls `quadratic(denom, cs[:,t], Qs[:,:,t])` (denom-first per RegisterUtilities v1). None of these are part of the API review scope but were necessary to get a green baseline.
+
+  Baseline tests: 200 pass / 0 fail / 0 error (Julia 1.12.6, 4 test groups: Register Optimize 148, Minimization to mismatch data 36, Optimization with a temporal penalty 12, Optimization with linear interpolation of mismatch data 4). Ambiguity count: 0. Test run emits a deprecation warning from Optim about `x_tol` — pre-existing, not addressed here.
+
+---
+
+### CHUNK-002: remove-dead-code
+- **Kind**: `implement`
+- **Originating finding**: Tier 1 / T1-D (`initial_deformation` Method 3, line 462) and T1-E (`optimize` function, line 1115)
+- **Cluster**: dead-code
+- **Breaking**: yes
+- **Description**: Remove two exported/semi-public methods that are already broken at runtime:
+  1. `initial_deformation(ap, cs, Qs, ϕ_old, maxshift)` (Method 3) — body immediately calls `error("This is broken, don't use it")`.
+  2. `optimize(tform::AffineMap, mmis, nodes)` — docstring says "not updated yet, probably broken"; uses old Optim API.
+  Remove both method definitions and any associated tests that exist solely to exercise the broken path. Update docstrings for `initial_deformation` to no longer mention Method 3.
+- **Depends on**: CHUNK-001
+- **Verification**: tests pass; `initial_deformation` still works for remaining methods; no references to the removed signatures remain
+- **Status**: `not-started`
+- **Notes**:
+
+---
+
+### CHUNK-003: stackidx-to-keyword
+- **Kind**: `implement`
+- **Originating finding**: Tier 1 / T1-A (`auto_λ` Method 3, line 906)
+- **Cluster**: none
+- **Breaking**: yes
+- **Description**: `auto_λ(stackidx::Integer, cs, Qs, nodes, mmis, λrange; kwargs...)` puts a configuration integer (`stackidx`) before data. Modern Julia convention is data-first.
+  Proposed: remove this overload as a separate method and instead add `stackidx` as a keyword to the relevant `auto_λ` overload(s):
+  `auto_λ(cs, Qs, nodes, mmis, λrange; stackidx=nothing, kwargs...)`.
+  When `stackidx` is not `nothing`, slice `cs`, `Qs`, `mmis` along the stack dimension before proceeding. Migrate or remove any tests for the old positional form.
+- **Depends on**: CHUNK-001
+- **Verification**: existing `auto_λ` calls without `stackidx` unaffected; new keyword form works; tests pass
+- **Status**: `not-started`
+- **Notes**:
+
+---
+
+### CHUNK-004: optimize-rigid-modernize
+- **Kind**: `implement`
+- **Originating finding**: Tier 1 / T1-B (line 81), Tier 2 / T2-A, T2-B
+- **Cluster**: rigid-search
+- **Breaking**: yes
+- **Description**: Three related changes to `optimize_rigid` (line 81):
+  1. **T1-B — positional-to-keyword**: `SD` and `maxrot` are currently positional arguments with defaults. Move both to keyword arguments:
+     `optimize_rigid(fixed, moving, A::AffineMap, maxshift; SD=I_matrix, maxrot=π, atol=1e-4, print_level=0, max_iter=3000, thresh=0, kwargs...)`
+     where `I_matrix` is constructed from `ndims(fixed)` at call time.
+  2. **T2-A — `tol` → `atol`**: Rename `tol` to `atol` (consistent with `Base.isapprox`, `IterativeSolvers`, etc.). No deprecation shim — clean rename.
+  3. **T2-B — `kwargs...` passthrough**: Replace the fixed keyword list (minus `atol`, `print_level`, `max_iter`, `thresh` which stay as explicit named keywords for discoverability) with a forwarded `kwargs...` so callers can pass any Ipopt option.
+  Update callers of `optimize_rigid` within the package (e.g., `rotation_gridsearch` calls it).
+- **Depends on**: CHUNK-001
+- **Verification**: `optimize_rigid` works with all-keyword call; `rotation_gridsearch` (which calls it) still works; tests pass
+- **Status**: `not-started`
+- **Notes**: `I_matrix` default for `SD` should be lazy — compute inside the function body as `Matrix{Float64}(I, ndims(fixed), ndims(fixed))`.
+
+---
+
+### CHUNK-005: rotation-gridsearch-SD-keyword
+- **Kind**: `implement`
+- **Originating finding**: Tier 1 / T1-C (`rotation_gridsearch`, line 191)
+- **Cluster**: rigid-search
+- **Breaking**: yes
+- **Description**: `rotation_gridsearch(fixed, moving, maxshift, maxradians, rgridsz, SD=...)` — `SD` is a positional argument with a default. Move `SD` to a keyword argument:
+  `rotation_gridsearch(fixed, moving, maxshift, maxradians, rgridsz; SD=I_matrix)`
+  where `I_matrix` is constructed lazily from `ndims(fixed)` inside the function body (as for CHUNK-004).
+- **Depends on**: CHUNK-004
+- **Verification**: existing calls without `SD` unaffected; explicit-SD calls updated to keyword form; tests pass
+- **Status**: `not-started`
+- **Notes**: Depends on CHUNK-004 because `rotation_gridsearch` calls `optimize_rigid`, and the `SD` default construction pattern should match.
+
+---
+
+### CHUNK-006: lambda-t-unify
+- **Kind**: `implement`
+- **Originating finding**: Tier 3 / T3-A — `λt` placement inconsistency across `initial_deformation` Method 4 (line 1052), `optimize!` Methods 3–4 (lines 730/743), `fixed_λ` Method 2 (line 817)
+- **Cluster**: lambda-t
+- **Breaking**: yes
+- **Description**: `λt` (the temporal penalty coefficient) appears at different positional slots in different functions:
+  - `initial_deformation(ap, λt, cs, Qs)` — position 2
+  - `optimize!(ϕs, ϕs_old, dp, λt, mmis)` — position 4
+  - `fixed_λ(cs, Qs, nodes, ap, λt, mmis)` — position 5
+
+  Unify by making `λt` a keyword argument across all three functions with a default of `nothing` (or `0`) meaning "no temporal penalty." This collapses the temporal and non-temporal overloads:
+  - `initial_deformation(ap, cs, Qs; λt=nothing)` — dispatch to `TimeHessian` internally when `λt !== nothing`
+  - `optimize!(ϕs, ϕs_old, dp, mmis; λt=nothing, kwargs...)` — use time-series path when `λt !== nothing`
+  - `fixed_λ(cs, Qs, nodes, ap, mmis; λt=nothing, ϕs_old=identity, mu_init=0.1, kwargs...)` — unified signature
+
+  Remove the now-redundant separate overloads. Update all internal callers. Update tests.
+- **Depends on**: CHUNK-001
+- **Verification**: single-timepoint calls (`λt` omitted) work; time-series calls (`λt=value`) work; ambiguity count unchanged or reduced; tests pass
+- **Status**: `not-started`
+- **Notes**: This is the most complex chunk — `TimeHessian` construction and dispatch currently happens via overload selection; it needs to move inside the function body as a conditional. Implement carefully with a test for each path.
+
+---
+
+### CHUNK-007: auto-lambda-t-interface
+- **Kind**: `decide`
+- **Originating finding**: Tier 3 / T3-B — `auto_λt` Method 1 (line 1019) accepts `λtrange`, Method 2 (line 1043) accepts scalar `λt`
+- **Cluster**: lambda-t
+- **Breaking**: no (decide chunk — no changes made until decision is recorded)
+- **Description**: `auto_λt` has two methods with different last-argument semantics:
+  - Method 1: `auto_λt(Es, cs, Qs, ap, λtrange)` — iterates over a range
+  - Method 2: `auto_λt(Es, cs::Array{Tf}, Qs::Array{Tf}, ap, λt)` — takes a scalar and repacks raw flat arrays
+
+  Method 2 appears to be a raw-format repacking helper (like the `Array{Tf}` overloads elsewhere). If it delegates to Method 1 with `λtrange = λt:λt` (a trivial range), the scalar `λt` here represents a fixed value, not a range to search — which makes calling `auto_λt` with a scalar semantically different from calling it with a range, and the function name `auto_λt` ("auto-select λt") doesn't make sense for a scalar input.
+
+  **Decision needed**: Should Method 2 (a) be renamed to reflect that it's a raw-format adapter, (b) be removed and callers updated to use Method 1 directly, or (c) keep as-is with improved documentation?
+- **Depends on**: CHUNK-006
+- **Verification**: depends on decision
+- **Status**: `not-started`
+- **Notes**: Record the decision in the `## Decisions` section above with this chunk ID, then the implementer will act on it.
+
+---
+
+### CHUNK-008: widen-array-types
+- **Kind**: `implement`
+- **Originating finding**: Tier 2 / T2-C (`auto_λ` Method 5, line 918) and Tier 3 / T3-C (`auto_λ` Method 4, line 913)
+- **Cluster**: none
+- **Breaking**: no
+- **Description**: Two related type-widening changes in legacy-format `auto_λ` overloads:
+  1. **T2-C** (line 918): `cs::Array{Float64}`, `Qs::Array{Float64}`, `mmis::Array{Float64}` — widen to `AbstractArray{Float64}` (or `AbstractArray{<:AbstractFloat}` if the downstream code tolerates it).
+  2. **T3-C** (line 913): `cs::Array{Tf}`, `Qs::Array{Tf}`, `mmis::Array{Tf}` where `Tf<:Number` — all three share the same `Tf`. Relax to independent type parameters or use `promote_type` inside the function body.
+  Neither change breaks existing callers (widening is non-breaking).
+- **Depends on**: CHUNK-001
+- **Verification**: existing tests pass; confirm `Float32` arrays are now accepted where they previously weren't
+- **Status**: `not-started`
+- **Notes**: Be careful about ambiguity with other overloads when widening — run `Test.detect_ambiguities` after.
+
+---
+
+### CHUNK-009: version-bump
+- **Kind**: `version-bump`
+- **Originating finding**: n/a
+- **Cluster**: none
+- **Breaking**: yes
+- **Description**: Bump version from 0.3.6 → 1.0.0 (first stable release, incorporating all breaking changes). Update CHANGELOG (or create one if absent). Tag the release.
+- **Depends on**: CHUNK-002, CHUNK-003, CHUNK-004, CHUNK-005, CHUNK-006, CHUNK-007, CHUNK-008
+- **Verification**: full test suite green; `Project.toml` version updated; no half-finished cluster; git tag created
+- **Status**: `not-started`
+- **Notes**: Only cut after all other chunks are complete and CHUNK-007 decision is resolved.
+
+## Dropped findings
+<!-- Items the user chose not to act on, with one-line reasons. Preserved for institutional memory. -->
+*(none — all findings approved)*
+
+## Session Log
+- 2026-05-18: Initial review and plan generated by /review-api. 9 chunks total (1 preflight, 6 implement, 1 decide, 1 version-bump).
+- 2026-05-18: Implemented CHUNK-001 (preflight). Baseline: 200/200 tests pass, 0 ambiguities at commit `7f64c5a`. Pre-existing uncommitted work (CI modernization, Register* compat bump to v1, and adapting two call sites to the v1 `penalty!` / `quadratic` signatures) was committed first to obtain a green baseline. Next up: CHUNK-002 (remove-dead-code).
+
+## Open Questions
+- CHUNK-007: How should the `auto_λt` scalar vs. range interface inconsistency be resolved? See chunk for options.

API_REVIEW_SESSION.md

@@ -0,0 +1,52 @@
+# Session Handoff — 2026-05-18
+
+## Plan
+API_REVIEW_PLAN.md — RegisterOptimize v0.3.6
+
+## What was just completed
+CHUNK-001: preflight
+Established the baseline. Working tree had unrelated in-progress work; landed
+three prep commits (CI/TagBot modernization, Register* compat bump to v1, and
+two-line adaptations of `TimeHessian` and a test for the v1 `penalty!` and
+`quadratic` signatures) before the test suite went green. Final baseline:
+200/200 tests pass, 0 ambiguities, at commit `7f64c5a`.
+
+## Key decisions / shim choices
+- User chose to commit the dirty working tree's changes rather than stash or
+  revert, then to fix forward against RegisterPenalty/RegisterUtilities v1
+  rather than pin to older deps.
+- The Optim `x_tol` deprecation warning is pre-existing and intentionally not
+  addressed in CHUNK-001.
+
+## State of the codebase
+- Files modified (since session start, across three commits):
+  `.github/workflows/CI.yml`, `.github/workflows/TagBot.yml`, `Project.toml`,
+  `src/RegisterOptimize.jl`, `test/register_optimize.jl`.
+- Test suite: 200/200 pass (Julia 1.12.6).
+- Ambiguity count: 0 (baseline).
+- Staged but uncommitted: no. Untracked: `.claude/`, `API_REVIEW_PLAN.md`,
+  `API_REVIEW_SESSION.md`.
+
+## Cluster status
+- (no cluster — preflight)
+- Remaining clusters: `dead-code` (1 chunk), `rigid-search` (2 chunks),
+  `lambda-t` (2 chunks), plus standalone CHUNK-003, CHUNK-008, CHUNK-009.
+
+## Next chunk
+CHUNK-002: remove-dead-code — remove `initial_deformation` Method 3 (line
+462, `error("This is broken…")`) and the broken `optimize(::AffineMap, …)`
+overload (line ~1115). Breaking change.
+
+## Watch out for
+- Baseline was reached only after fixing-forward to RegisterPenalty v1 and
+  RegisterUtilities v1. Other call sites of `penalty!` (lines 442, 449, 549,
+  556, 600, 612, 708, 714, 762, 767, 965, 969, 987, 991, 1174) all use the
+  longer signatures and were unaffected — but any future change in the v1
+  signatures could break them.
+- CHUNK-006 (lambda-t-unify) touches the same `TimeHessian` machinery whose
+  arg-order was just fixed. Double-check that `penalty!(y, ϕs, λt)` continues
+  to apply when `λt` becomes optional.
+- Julia 1.12 is in use locally; CI matrix is `min` (= 1.10 per Project.toml
+  `julia` compat) and `1` (latest). The local Manifest currently resolves
+  Stdlib versions to 1.11 / 1.12 — this is fine for testing but worth
+  remembering if results ever diverge.

CHANGELOG.md

@@ -0,0 +1,63 @@
+# Changelog
+
+## v1.0.0
+
+**Breaking changes**
+
+- `initial_deformation(ap, cs, Qs, ϕ_old, maxshift)` removed. The method body was
+  `error("This is broken, don't use it")` and is gone entirely.
+
+- `optimize(tform::AffineMap, mmis, nodes)` removed. Used a deprecated Optim API
+  (`interior`/`DifferentiableFunction`/`At_mul_Bt!`) that no longer exists.
+
+- `auto_λ`: the `stackidx::Integer` positional-first-argument overload is removed.
+  Pass `stackidx` as a keyword instead:
+  ```julia
+  # before
+  auto_λ(stackidx, cs, Qs, nodes, mmis, λrange)
+  # after
+  auto_λ(cs, Qs, nodes, mmis, λrange; stackidx)
+  ```
+
+- `optimize_rigid`: `SD` and `maxrot` are now keyword arguments (no defaults
+  change); `tol` is renamed `atol`; additional keyword arguments are forwarded
+  to Ipopt:
+  ```julia
+  # before
+  optimize_rigid(fixed, moving, tform0, maxshift, SD, maxrot; tol=1e-4)
+  # after
+  optimize_rigid(fixed, moving, tform0, maxshift; SD, maxrot, atol=1e-4)
+  ```
+
+- `rotation_gridsearch`: `SD` is now a keyword argument:
+  ```julia
+  # before
+  rotation_gridsearch(fixed, moving, maxshift, maxradians, rgridsz, SD)
+  # after
+  rotation_gridsearch(fixed, moving, maxshift, maxradians, rgridsz; SD)
+  ```
+
+- `initial_deformation`, `optimize!` (time-series), and `fixed_λ`: the temporal
+  penalty coefficient `λt` is now a keyword argument (default `nothing`):
+  ```julia
+  # before
+  initial_deformation(ap, λt, cs, Qs)
+  optimize!(ϕs, ϕs_old, dp, λt, mmis)
+  fixed_λ(cs, Qs, nodes, ap, λt, mmis)
+  # after
+  initial_deformation(ap, cs, Qs; λt)
+  optimize!(ϕs, ϕs_old, dp, mmis; λt)
+  fixed_λ(cs, Qs, nodes, ap, mmis; λt)
+  ```
+
+- `auto_λt` flat-array adapter overload removed. Pass SVector/SMatrix-typed
+  `cs`/`Qs` directly to the main `auto_λt(Es, cs, Qs, ap, λtrange)` overload.
+
+**Non-breaking improvements**
+
+- `auto_λ` flat-array overloads now accept `AbstractArray{<:Number}` and
+  `AbstractArray{Float64}` instead of requiring `Array{Tf}` / `Array{Float64}`,
+  allowing views and other array wrappers.
+
+- `auto_λ` flat-array overload: the element types of `cs`, `Qs`, and `mmis` are
+  now independent (previously all three had to share the same `Tf`).

Project.toml

@@ -1,7 +1,7 @@
 name = "RegisterOptimize"
 uuid = "b981a1b5-4587-53b0-9c3d-454c648f6f8d"
 authors = ["Tim Holy <tim.holy@gmail.com>"]
-version = "0.3.6"
+version = "1.0.0"
 
 [deps]
 CachedInterpolations = "b9709bfb-d23d-5560-8276-8c35c4b76823"
@@ -27,8 +27,8 @@ StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
 Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
 
 [compat]
-CachedInterpolations = "0.1"
-CenterIndexedArrays = "0.2"
+CachedInterpolations = "1"
+CenterIndexedArrays = "1"
 CoordinateTransformations = "0.5, 0.6"
 ForwardDiff = "0.10, 1"
 Interpolations = "0.9, 0.10, 0.11, 0.12, 0.13, 0.14, 0.15, 0.16"
@@ -39,10 +39,10 @@ MathOptInterface = "1"
 NLsolve = "3, 4"
 Optim = "0.18, 0.19, 0.20, 1"
 ProgressMeter = "0.7, 0.8, 0.9, 1"
-RegisterCore = "0.2"
-RegisterDeformation = "0.3, 0.4"
-RegisterFit = "0.1, 0.2"
-RegisterPenalty = "0.1, 0.2, 0.3"
+RegisterCore = "1"
+RegisterDeformation = "1"
+RegisterFit = "1"
+RegisterPenalty = "1"
 StaticArrays = "0.10, 0.11, 0.12, 1"
 Statistics = "1"
 julia = "1.10"