(breaking): new AbstractSide API replacing old Symbol-based API#41
Merged
(breaking): new AbstractSide API replacing old Symbol-based API#41
AbstractSide API replacing old Symbol-based API#41Conversation
…rchy Replace :left/:right/:nearest Symbols and Val-based dispatch with proper type hierarchy (AbstractSide → NearestSide, LeftSide, RightSide), mirroring the recent AbstractExtrap refactoring. Removed: SideVal alias, @_dispatch_side/@_dispatch_side_nd macros, _validate_side, _is_uniform_side, _to_side_val/_to_side_vals helpers, and 6 manual if-else dispatch blocks in integration code. BREAKING CHANGE: side kwarg now requires AbstractSide instances instead of Symbols (e.g. side=NearestSide() instead of side=:nearest).
…ide dispatch
Replace Val{:left}/Val{:right} Symbol-based dispatch with LeftSide/RightSide
type hierarchy across all polyfit kernel functions and their callers:
- _compute_deriv1: 14 specialized methods + generic fallback
- _compute_deriv1_coeffs: 8 specialized methods + generic fallback
- _compute_deriv1_coeffs!: generic barycentric fallback
- _extract_stencil_values: 2 methods
- _estimate_endpoint_derivative: 4 methods
- materialize_bc: signature + 3 passthrough methods
- Callers in cubic_solver, cubic_nd_math, quadratic_solver
mgyoo86
changed the title
AbstractSide API replacing old Symbol-based API
There was a problem hiding this comment.
Pull request overview
This PR completes the migration away from Symbol-dispatched side selection for constant interpolation by introducing a typed AbstractSide hierarchy (NearestSide(), LeftSide(), RightSide()) and threading it through 1D/ND constant interpolation, PolyFit endpoint derivative estimation, and integration/show paths.
Changes:
- Introduce and export
AbstractSide+ concrete side tags; replace all:left/:right/:nearest(andVal(...)) dispatch for constant interpolation with typed dispatch. - Update constant interpolation evaluation + ND utilities + integration kernels/APIs to accept/store typed side tags (including per-axis tuples for ND).
- Update docs and extensive tests to the new typed side API (including show formatting expectations).
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_type_stability.jl | Removes outdated note tied to removed side-dispatch macro in ND tests. |
| test/test_show.jl | Updates show output expectations from :left/:nearest to LeftSide/NearestSide. |
| test/test_polyfit_bc.jl | Migrates PolyFit endpoint-derivative tests from Val(:left/:right) to LeftSide()/RightSide(). |
| test/test_nd_utils_shared.jl | Updates _resolve_side_nd tests to typed sides; expects MethodError for Symbols. |
| test/test_nd_coverage.jl | Updates mixed-side ND constant interpolation coverage to typed side tuples. |
| test/test_nd_constant.jl | Updates ND constant interpolation behavioral tests to typed side keywords/tuples. |
| test/test_integral_series.jl | Updates constant series integration tests to typed sides. |
| test/test_integral_nd_exactness.jl | Updates ND integration exactness/finite tests to typed sides. |
| test/test_integral_nd.jl | Updates ND integration smoke tests to typed side tuples. |
| test/test_integral_fulldomain.jl | Updates full-domain integration tests to typed sides (1D/series/ND). |
| test/test_integral_extrap.jl | Updates constant extrap integration tests to typed sides. |
| test/test_integral_allocation.jl | Updates allocation tests to typed sides. |
| test/test_integral_1d.jl | Updates 1D integration tests to typed sides. |
| test/test_derivatives.jl | Updates constant-kernel derivative test callsite to typed side. |
| test/test_cumulative_integrate.jl | Updates cumulative integration tests to typed sides. |
| test/test_constant_series_interp.jl | Updates constant series API tests; verifies side is preserved as the side tag value. |
| test/test_constant_anchor.jl | Updates anchored-query constant interpolation tests to typed sides. |
| test/test_constant.jl | Removes macro/SideVal tests; adds AbstractSide hierarchy tests; updates invalid-side expectations. |
| test/test_complex_constant_series.jl | Updates complex constant series side-option tests to typed sides. |
| test/test_complex_constant.jl | Updates complex constant interpolation tests to typed sides. |
| test/test_coeffs.jl | Updates constant coeffs tests to typed sides. |
| test/test_autodiff_Zygote.jl | Updates Zygote constant interpolation tests to typed sides. |
| test/test_autodiff_ForwardDiff.jl | Updates ForwardDiff constant interpolation tests to typed sides. |
| test/test_autodiff_Enzyme.jl | Updates Enzyme constant interpolation tests to typed sides. |
| src/quadratic/quadratic_solver.jl | Switches PolyFit BC materialization endpoint tags to typed sides. |
| src/integral/integrate_kernels.jl | Replaces side dispatch in constant integral kernels and ND weights with typed sides. |
| src/integral/integrate_fulldomain.jl | Simplifies constant integration paths by passing typed side directly (no manual branch). |
| src/integral/integrate_api.jl | Simplifies constant integration API by passing typed side directly (no manual union-avoidance branch). |
| src/cubic/nd/cubic_nd_math.jl | Switches endpoint derivative estimation to typed sides. |
| src/cubic/cubic_solver.jl | Switches PolyFit BC materialization endpoint tags to typed sides. |
| src/core/utils.jl | Deletes _dispatch_side and _dispatch_side_nd macros. |
| src/core/show.jl | Updates side formatting to typed sides. |
| src/core/polyfit_kernels.jl | Migrates endpoint derivative kernels from Val(:left/:right) to typed sides. |
| src/core/nd_utils.jl | Replaces symbol-based _resolve_side_nd with typed-side version. |
| src/core/eval_ops.jl | Defines AbstractSide + NearestSide/LeftSide/RightSide types. |
| src/core/bc_types.jl | Updates materialize_bc endpoint parameter to typed side tags. |
| src/constant/nd/constant_nd_types.jl | Updates ND constant interpolant side tuple type parameter to typed side tags. |
| src/constant/nd/constant_nd_oneshot.jl | Updates ND oneshot APIs to accept typed sides and removes side-dispatch macro usage. |
| src/constant/nd/constant_nd_interpolant.jl | Updates ND constructor to accept typed sides and removes Val conversion helpers. |
| src/constant/nd/constant_nd_eval.jl | Updates generated ND kernel to accept typed side tuples and offset helpers to dispatch on types. |
| src/constant/constant_types.jl | Stores side as a type parameter SD<:AbstractSide; constructor now accepts typed side tags. |
| src/constant/constant_series_interp.jl | Stores series side as SD<:AbstractSide; updates internal signatures to accept typed sides. |
| src/constant/constant_oneshot.jl | Updates 1D oneshot/in-place APIs to accept side::AbstractSide and removes side macro dispatch. |
| src/constant/constant_kernels.jl | Migrates _constant_kernel dispatch from Val to typed side tags. |
| src/constant/constant_interpolant.jl | Updates callable interpolant constructor signature/docs to typed sides. |
| src/FastInterpolations.jl | Exports AbstractSide, NearestSide, LeftSide, RightSide. |
| docs/src/interpolation/constant.md | Updates constant interpolation user docs/examples to typed sides. |
| docs/src/api/constant.md | Documents typed side selection types and updates API examples. |
| README.md | Updates constant interpolation examples to typed sides; aligns extrap examples with typed extrap API. |
Comments suppressed due to low confidence (2)
src/core/eval_ops.jl:161
- The docstring claims a "Union-splitting guarantee" based on there being exactly 3
AbstractSidesubtypes. In this implementation, performance mostly comes from storingsideas a concrete type parameter (andTuples of concrete side types), not from union-splitting; also,AbstractSideis extensible so the “exactly 3 subtypes” assumption can be invalidated. Consider rewording this section to describe the actual mechanism (parametric/typed dispatch) and/or documenting that onlyNearestSide/LeftSide/RightSideare supported.
!!! info "Union-splitting guarantee"
With exactly 3 concrete subtypes (< 4 limit), Julia union-splits
automatically on hot paths. No manual dispatch macros needed.
test/test_polyfit_bc.jl:1815
- The allocation reporting assigns
alloc_d4andalloc_d5twice in a row with identical calls. This looks like an accidental duplication (the first values are overwritten immediately) and can be removed or replaced with distinct variables if two measurements are intended (e.g., warmup vs measured).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…aluation and polyfit kernels
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41 +/- ##
==========================================
- Coverage 97.84% 97.79% -0.06%
==========================================
Files 71 71
Lines 5944 5852 -92
==========================================
- Hits 5816 5723 -93
- Misses 128 129 +1
🚀 New features to boost your workflow:
|
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.
Summary
Replace the Symbol-based
sideAPI (:nearest,:left,:right) in constant interpolation with a typedAbstractSidehierarchy (NearestSide(),LeftSide(),RightSide()). This mirrors theAbstractExtrapmigration completed in the previous PR and eliminates the last Symbol-dispatched API surface in the package.This is a breaking change for any code using
side=:nearest,side=:left, orside=:right.Deleted Code
@_dispatch_sidemacroValbranching at call sites_validate_side(::Symbol)SideValtype aliasUnion{Val{:nearest}, Val{:left}, Val{:right}}integrate_api.jl_to_side_val(::Symbol)Net: 49 files changed, −199 lines (663 insertions, 862 deletions)
New Exports
export AbstractSide, NearestSide, LeftSide, RightSideMigration Guide
Impact
_compute_deriv1,_estimate_endpoint_derivative) useLeftSide/RightSideinstead ofVal{:left}/Val{:right}