Skip to content

API review: argument order, type widening, constructor cleanup#23

Merged
kdw503 merged 7 commits into
masterfrom
dwk/mng
May 15, 2026
Merged

API review: argument order, type widening, constructor cleanup#23
kdw503 merged 7 commits into
masterfrom
dwk/mng

Conversation

@kdw503
Copy link
Copy Markdown
Member

@kdw503 kdw503 commented May 15, 2026

Summary

  • Fix temporal penalty!/penalty argument order — swap (g, λt, ϕs)(g, ϕs, λt) to match Julia's data-first convention (CHUNK-003)
  • Widen type annotationsVector{D<:GridDeformation}AbstractVector{<:GridDeformation> in temporal penalty methods; Array{T}AbstractArray{T} in vec2ϕs (CHUNK-004, CHUNK-006)
  • Add nothing as alias for identity in penalty!(g, ϕ, ϕ_old, ...) — self-documenting way to say "no prior deformation" (CHUNK-005)
  • Hide AffinePenalty sentinel constructor — replace (F, λ, _) dummy-arg form with clean (F, λ) inner constructor; update Base.convert accordingly (CHUNK-007)
  • Simplify eltype/ndims dispatch chain — collapse 6-method supertype-delegation chain to 4 methods using ::Type{<:DeformationPenalty{T,N}} wildcard (CHUNK-008)
  • Narrow compat bounds — drop pre-v1 entries for HolyLab dependencies now at v1

Test plan

  • All 7 test suites pass (Pkg.test()): Doctests, Aqua, ExplicitImports, Affine penalty, Data penalty, Total penalty, Temporal penalty
  • Test.detect_ambiguities(RegisterPenalty) == 0 (verified throughout)
  • penalty!(g, ϕs, λt) and penalty(ϕs, λt) round-trip correctly against ForwardDiff
  • penalty!(g, ϕ, nothing, dp, mmis) produces identical values and gradients to identity form
  • vec2ϕs callable with a view argument

🤖 Generated with Claude Code

kdw503 and others added 5 commits May 15, 2026 09:59
…overload

CHUNK-003: swap λt/ϕs argument order in temporal penalty! and penalty to
match Julia's data-first convention.

CHUNK-004: widen Vector{D<:GridDeformation} to AbstractVector{<:GridDeformation}
in temporal penalty! and penalty, dropping the unused D type variable.

CHUNK-005: add penalty!(g, ϕ, ::Nothing, ...) overloads as a self-documenting
alias for identity ("no prior deformation").

CHUNK-006: widen vec2ϕs x::Array{T} to x::AbstractArray{T}, consistent with
the downstream convert_to_fixed signature; add view-based test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace AffinePenalty{T,N}(F, λ, _) dummy-arg inner constructor with a clean
2-arg form AffinePenalty{T,N}(F, λ) calling new directly, and update
Base.convert to drop the dummy argument.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collapse the 6-method eltype/ndims chain to 4 methods. Replace the
exact-type + supertype-delegation pair with a single ::Type{<:DeformationPenalty{T,N}}
wildcard overload for each, preserving the two instance-dispatch wrappers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CachedInterpolations, CenterIndexedArrays, RegisterCore, RegisterDeformation,
and RegisterUtilities have all released v1; drop pre-1.0 compat entries.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.03%. Comparing base (410357c) to head (8516811).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   98.55%   99.03%   +0.48%     
==========================================
  Files           1        1              
  Lines         208      208              
==========================================
+ Hits          205      206       +1     
+ Misses          3        2       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kdw503 and others added 2 commits May 15, 2026 10:27
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tests

CHUNK-007 replaced AffinePenalty{T,N}(F, λ, _) with a 2-arg (F::Matrix{T},
λ::T) constructor, which inadvertently shadowed the (nodes::AbstractMatrix, λ)
inner constructor — Matrix{T} is more specific than AbstractMatrix, so
AffinePenalty(nodes_mat, 1.0) silently stored the raw node coordinates as F
without computing QR. Fix by restoring a typed sentinel constructor
(F, λ, Val(:_precomputed)) used only by Base.convert, removing the ambiguous
2-arg form.

Tests added:
- AffinePenalty(AbstractMatrix, λ): assert dp.F is orthonormal (catches the bug)
- AffinePenalty(Vector{AbstractVector}, λ): type check
- penalty!(g, ϕ, mmis) with mismatched gradient length: checks the error message

Note: itptype(::Type{Linear}) / interpolate_mm!(mm, Linear) is a pre-existing
broken path (Gridded interpolation does not support interpolate!); left as-is.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@kdw503 kdw503 merged commit 69d8559 into master May 15, 2026
5 checks passed
@kdw503 kdw503 deleted the dwk/mng branch May 15, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant