feat: Typed Autocache with Generic Boundary Conditions#3
Merged
Conversation
- Add bc_types.jl with AbstractBC, D1, D2, DerivativeBCData types - Enable asymmetric BC: bc=(D1(slope), D2(curvature)) - Support :clamped symbol mapping to D1(0), D1(0) - Remove bc::Symbol type restriction from cubic_interp API - Fix bug where D1/D2 BC was ignored (always using :natural cache) - Unify natural BC to use DerivativeBCData internally - Simplify _build_cache with parametric dispatch - Maintain zero-allocation for :natural/:periodic autocache
- Test BC type construction (D1, D2 with Float64/Float32) - Verify symbol equivalence (:natural == D2(0), :clamped == D1(0)) - Mathematical correctness: polynomials (linear, quadratic, cubic) reproduced exactly within 1e-14 tolerance - Test CubicSplineCache and CubicInterpolant with D1/D2 - Float32 support and edge cases - Use rtol/atol constants for consistent tolerance management
- Move bc_types.jl include before utils.jl (fixes UndefVarError: AbstractBC) - Consolidate _validate_bc functions in utils.jl - Add :clamped to valid BC symbols
BC API now accepts: - Symbol: :natural, :clamped, :periodic - BCPair: BCPair(D1(0.5), D2(1.0)) - PointBC: D1(0.5) → symmetric BCPair - PeriodicBC: PeriodicBC() Key changes: - _normalize_bc returns BCPair instead of Tuple - Remove Tuple validation from _validate_bc - Update CubicSplineCache to use BCPair fields - Rename DerivativeBCData → BCPair - Add type aliases: NaturalBCPair, ClampedBCPair - Add PeriodicBC as user-facing type - Add PointBC as abstract parent of D1, D2 BREAKING CHANGE: bc=(D1(0.5), D2(1.0)) no longer supported Use: bc=BCPair(D1(0.5), D2(1.0))
- Add `bc::Union{Symbol,AbstractBC}=:natural` to all public API functions
- Standardize comments explaining BC dispatch logic:
- Periodic: both :periodic symbol and PeriodicBC type
- Autocache: only for :natural symbol
- Fresh cache: :clamped, PointBC, BCPair
- Remove redundant PeriodicBC check after normalize (already checked before)
- Update CubicSplineCache constructor with consistent pattern
…BC combination
- Replace manual type aliases with fully parametric CacheEntry{T,L,R,X} and CacheBank{T,L,R,X}
- Add dynamic bank registry via IdDict with lock-free read path
- Enable autocache for all BCPair types, not just :natural
- Add core helper functions (_cubic_interp_bcpair!, _cubic_interp_periodic!, etc.)
- Use BC value override pattern: cache by BC types, apply values at solve time
- Rename stats.size to stats.total_entries for clarity
- Replace lock() do closure with try-finally to avoid closure allocation - Consolidate _pass1_identity_check + _lookup_or_insert_locked! into single _lookup_or_insert! - Skip range normalization for already StepRangeLen inputs (from range(a,b,n)) - Verified: cache hit = 0 bytes, cache miss = ~6KB (LU factorization)
- Add helper functions: _is_periodic_bc, _get_cache_and_solve!, _get_cache_and_solve_periodic! - Simplify 4-arg cubic_interp! using helpers to reduce code duplication - Create cubic_interpolant.jl with: - CubicInterpolant callable methods (scalar, vector, in-place) - 2-arg cubic_interp(x, y; bc, extrap, autocache) API - _build_interpolant_bcpair and _build_interpolant_periodic helpers - Update include order in FastInterpolations.jl Note: Periodic BC has ~2x performance regression vs master (714ns vs 340ns) that needs investigation in follow-up work.
- Remove Union{Symbol,AbstractBC} from all public APIs
- Remove _symbol_to_bc() and _validate_bc(::Symbol) helpers
- Remove Val(:natural)/Val(:periodic) deprecated autocache API
- Update all tests to use NaturalBC(), ClampedBC(), PeriodicBC()
- Simplify _normalize_bc to handle AbstractBC types only
BREAKING CHANGE: bc=:natural/:periodic/:clamped no longer supported,
use bc=NaturalBC()/PeriodicBC()/ClampedBC() instead
- Add tests for BCPair with dynamically changing derivative values - Verify cache hit when BC TYPE is same but VALUES differ - Test scalar, vector, loop accumulation scenarios - Cover D2-D1, D2-D2, D1-D1 type combinations - Include Float32 variant
- Reduce type params from {T,C,Y,Z,E} to {T,C} for type stability
- Add ExtrapVal union type for union-splitting optimization
- Replace @_dispatch_extrap macro with _symbol_to_extrap_val helper
- Remove double z copy (pass z_workspace, constructor copies once)
- Constructor now copies y,z to Vector{T} for consistent typing
Benefits:
- 2-arg cubic_interp(x,y) now passes @inferred (type stable)
- All extrap values return same CubicInterpolant type
- 480 bytes saved per CubicInterpolant creation (1632→1152)
- Scalar call remains zero-allocation (~140ns)
- Add test_type_stability.jl with 57 @inferred tests covering: - 2-arg form (CubicInterpolant) for all BC types - extrap variations returning consistent types - BC + extrap combinations - Cache-based API type stability - Float32 and Range vs Vector inputs - linear_interp type stability - Rename test_cubic_callable.jl → test_cubic_interpolant.jl - Remove unused NaturalBCPair/ClampedBCPair type aliases (replaced by NaturalBC/ClampedBC types)
- Remove unused _eval_with_bc and _cubic_vector_loop! for Nothing BC type (BCPair and PeriodicData are the only valid bc_data types now) - Add coverage tests for get_cubic_cache ClampedBC and PointBC typed APIs - Add coverage tests for Int Vector/Range fallback paths in autocache - Add coverage tests for periodic cache self-healing path - Add BC type conversion and _normalize_bc coverage tests
Improved comment explaining why y and z are always copied: - Ensures interpolant immutability after construction - Guarantees identical results for same query - Prevents silent corruption from external modifications
Replace floor(Int, ...) with unsafe_trunc(Int, ...) in _find_interval_with_bounds for Range grids. This is safe because: 1. xi is validated by _check_domain before reaching this point 2. clamp handles edge cases from floating point arithmetic Benchmark results: - _find_interval_with_bounds: 5.5 ns → 3.5 ns (37% faster)
Document caller requirements for _find_interval_with_bounds: - xi must be validated by _check_domain - step(x) > 0 (ascending grid) - NaN/Inf is undefined behavior (caller's responsibility) Move safety rationale from inline comment to docstring.
…tion Remove explicit extrapolation=Extension to use default (none) for fair comparison with FastInterpolations defaults.
Prevent division by zero if (1 + v'q) ≈ 0, which would indicate a degenerate periodic grid. Provides detailed error message with diagnostic values (denom, α, q[1], q[n]) for debugging. Performance impact: negligible (~0.3ns, branch prediction success).
- cubic_autocache.jl: Document why registry uses Any type, add non-atomic statistics warning, explain zero BC placeholders with reference to 3-arg _solve_system! call site - cubic_interpolant.jl: Add warning about get_cubic_cache returning placeholder zeros in bc_data - cubic_solver.jl: Document 2-arg vs 3-arg _solve_system! overloads and their respective use cases (manual cache vs autocache)
- Rename get_cubic_cache → _get_cubic_cache (internal API) - Remove 2-arg _solve_system!(cache, y) convenience dispatch for BCPair - All callers now explicitly pass bc_data: _solve_system!(cache, y, cache.bc_data) - Add 3-arg PeriodicData version (ignores bc_data, for API consistency) - Pass BCPair directly instead of tuple unpacking/repacking - Store actual BC values on cache miss (not placeholder zeros) This makes the solver API explicit and consistent, simplifying future refactoring.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3 +/- ##
==========================================
+ Coverage 98.72% 98.97% +0.25%
==========================================
Files 8 10 +2
Lines 627 781 +154
==========================================
+ Hits 619 773 +154
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the cubic spline interpolation system from symbol-based boundary conditions (:natural, :periodic) to a fully parametric type hierarchy with generic boundary conditions. The refactor introduces compile-time BC type specialization while maintaining BC value flexibility at runtime.
Key Changes:
- New
AbstractBC{T}type hierarchy withD1,D2,BCPair,NaturalBC,ClampedBC, andPeriodicBCtypes - Parametric
CacheBank{T,L,R,X}structure enabling cache sharing based on BC types (not values) - Separated 2-arg API into dedicated
cubic_interpolant.jlfile - Performance:
unsafe_truncfor ~37% faster uniform grid lookups, Sherman-Morrison robustness check - Type-stable interpolant construction with immutable y/z copies
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bc_types.jl | New BC type hierarchy with normalization and promotion helpers |
| src/cubic_autocache.jl | Refactored to parametric cache banks with dynamic registry |
| src/cubic_solver.jl | Generic BC solvers using type dispatch for D1/D2 combinations |
| src/cubic_interp.jl | Unified 4-arg API with BC normalization and core implementations |
| src/cubic_interpolant.jl | Separated 2-arg API and CubicInterpolant callable methods |
| src/cubic_types.jl | Simplified CubicInterpolant type parameters, ExtrapVal union |
| src/cubic_eval.jl | BC-aware dispatch methods for evaluation |
| src/utils.jl | Performance: unsafe_trunc, BC validation for new types |
| src/FastInterpolations.jl | Updated exports for BC types |
| test/*.jl | Comprehensive updates from symbol-based to typed BC API |
Comments suppressed due to low confidence (1)
test/test_cubic_interpolant.jl:18
- The test assertion has changed from
===(identity check) to==(equality check) to accommodate the fact thatCubicInterpolantnow always copies theyvector in its constructor for immutability.
While the comment explains this is intentional, this represents a subtle behavior change: the interpolant no longer shares memory with the input y vector. This could impact performance for large arrays and breaks user expectations if they were relying on shared memory (though that would be poor practice).
Consider documenting this behavior change in the PR description's breaking changes section if not already included.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change denominator check from eps(T) to sqrt(eps(T)) for better numerical stability (~1e-8 for Float64 vs ~2e-16) - Catches near-degenerate cases before precision loss occurs - Add tolerance value to error message for debugging - Remove get_cubic_cache from public exports (internal API only)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Typed Autocache with Generic Boundary Conditions
Summary
Refactors the caching system to use fully parametric types and introduces a comprehensive boundary condition type hierarchy for type-stable, zero-allocation cubic spline interpolation.
Key Changes
AbstractBC{T}hierarchy withD1,D2,BCPair,NaturalBC,ClampedBC,PeriodicBCCacheBank{T,L,R,X}enables compile-time specialization for any BC combinationcubic_interp(x,y)→CubicInterpolant) moved to dedicated fileunsafe_truncfor ~37% faster interval finding on uniform gridsBreaking Changes
bc=:naturalbc=NaturalBC()bc=:periodicbc=PeriodicBC()bc=(left_val, right_val)bc=BCPair(D1(left), D2(right))New API Examples
Test Plan
@inferredtestsStats