(refac): Relax periodic endpoint check + PeriodicBC(check=false) opt-out#94
Open
(refac): Relax periodic endpoint check + PeriodicBC(check=false) opt-out#94
PeriodicBC(check=false) opt-out#94Conversation
Three-tier dispatch for _check_periodic_endpoints:
- AbstractFloat / Complex{<:AbstractFloat}: isapprox with atol=8eps(T)
(compile-time folded, zero overhead). Covers both rtol-relative diffs
and near-zero noise floor (sin(0) vs sin(2π)).
- Other _PromotableValue (Integer, Rational): isapprox default tolerances.
- Duck types: strict == (isapprox semantics not guaranteed).
Add `check::Bool` field to PeriodicBC (default true). When false, skips _check_periodic_endpoints entirely — useful for scaled periodic data (e.g. 1e6*sin.(x)) where noise exceeds the 8eps atol floor. Also adds atol=8eps(T) noise floor for AbstractFloat/Complex endpoints, covering direct evaluations like sin.(x) where endpoints are near zero. Error message now mentions check=false as an escape hatch. Three-tier endpoint validation: - AbstractFloat/Complex: isapprox with atol=8eps(T) (compile-time folded) - Other _PromotableValue: isapprox default tolerances - Duck types: strict ==
…singleton
check::Bool field made PeriodicBC sizeof 1 (was 0), causing 48-byte
allocation regression in ND one-shot paths. Move to type parameter C
so PeriodicBC{E, P, C} remains a zero-size singleton when period is
Nothing, matching pre-change behavior.
Replace == with ≈ in all periodic BC documentation. Document v0.4.0–v0.4.5 strict equality vs v0.4.6+ isapprox behavior. Add check=false and scaled data guidance.
Write LocalPreferences.toml before test runs to activate pool bounds checking during CI. Applies to both main test and extension test jobs.
Contributor
FastInterpolations.jl BenchmarksAll benchmarks (42 total, click to expand)
This comment was automatically generated by Benchmark workflow. |
There was a problem hiding this comment.
Pull request overview
Relaxes periodic endpoint validation for cubic splines by switching from strict equality to isapprox with an atol = 8eps(T) noise floor, and introduces an opt-out via PeriodicBC(check=false) to skip endpoint validation for scaled/noisy periodic signals.
Changes:
- Implement type-dispatched periodic endpoint checks (
AbstractFloat/Complexuseisapproxwithatol=8eps, others keep strict==) and addPeriodicBC(check=false)(type parameter) to bypass validation. - Update 1D and ND cubic build paths to honor the new
checkflag; adjust error messages and docs to describe the relaxed behavior. - Expand tests to cover near-zero tolerance behavior and the
check=falseopt-out; tweak an ND allocation threshold test and enable AdaptiveArrayPools runtime checks in CI.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/bc_types.jl |
Adds check::Bool as a type parameter to PeriodicBC and a periodic_check accessor. |
src/core/periodic.jl |
Implements new endpoint validation dispatch + check=false gating; updates error message text. |
src/cubic/cubic_oneshot.jl |
Threads bc into _check_periodic_endpoints so check=false is honored in one-shot periodic solve. |
src/cubic/cubic_interpolant.jl |
Threads bc into _check_periodic_endpoints during periodic interpolant build. |
src/cubic/nd/cubic_nd_build.jl |
Updates ND periodic data validation to use isapprox/== based on element type and gates checks on periodic_check. |
test/test_periodic_bc.jl |
Adds coverage for relaxed endpoint checks, scaled-data failure, and PeriodicBC(check=false) + @inferred type stability. |
test/test_cubic_nd_oneshot.jl |
Loosens a mixed-BC allocation assertion with an explanatory comment. |
docs/src/migration/to_v0.4.md |
Documents the change from strict equality to isapprox and mentions check=false for scaled data. |
docs/src/interpolation/cubic.md |
Updates PeriodicBC documentation to reflect ≈ requirement. |
docs/src/boundary-conditions/periodicbc.md |
Updates periodic data requirement and adds guidance for scaled data / check=false. |
docs/src/boundary-conditions/overview.md |
Updates the PeriodicBC row to reflect ≈ rather than ==. |
.github/workflows/CI.yml |
Enables AdaptiveArrayPools runtime checks during CI test runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+261
to
+267
| elseif Tv <: Complex && Tv.parameters[1] <: AbstractFloat | ||
| RT = Tv.parameters[1] | ||
| check = quote | ||
| v1 = @inbounds data[$(first_idx...)] | ||
| vn = @inbounds data[$(last_idx...)] | ||
| isapprox(v1, vn; atol = 8 * eps($RT)) || _throw_periodic_nd_error($D, v1, vn) | ||
| end |
There was a problem hiding this comment.
In the @generated periodic ND validation, the branch elseif Tv <: Complex && Tv.parameters[1] <: AbstractFloat is unsafe: if Tv is a UnionAll/abstract type (e.g. an AbstractArray{Complex} with eltype == Complex), Tv.parameters will throw during code generation. Use a subtype check that doesn’t require accessing .parameters (or guard with isconcretetype(Tv) / Tv isa DataType) before indexing the parameter, so this path can’t error at compile time.
Comment on lines
+106
to
+108
| "Tip: set y[end] = y[1] explicitly, use " * | ||
| "PeriodicBC(endpoint=:exclusive) if your data does not repeat the first point, " * | ||
| "or PeriodicBC(check=false) to skip this validation." |
There was a problem hiding this comment.
The endpoint mismatch error message always suggests PeriodicBC(check=false), but _throw_periodic_endpoint_error is also reachable from call sites that don’t have a PeriodicBC in scope (e.g. cache-based APIs that call _check_periodic_endpoints(y) directly). In those cases the tip isn’t actionable and may confuse users. Consider either (a) only mentioning check=false when the caller passes a PeriodicBC, or (b) updating the relevant call sites/APIs to thread the PeriodicBC through so the opt-out is actually available everywhere this error can be raised.
Suggested change
| "Tip: set y[end] = y[1] explicitly, use " * | |
| "PeriodicBC(endpoint=:exclusive) if your data does not repeat the first point, " * | |
| "or PeriodicBC(check=false) to skip this validation." | |
| "Tip: set y[end] = y[1] explicitly, or use " * | |
| "PeriodicBC(endpoint=:exclusive) if your data does not repeat the first point." |
Comment on lines
+208
to
215
| struct PeriodicBC{E, P, C} <: AbstractBC | ||
| period::P # Nothing or AbstractFloat | ||
| function PeriodicBC{E, P}(period::P) where {E, P} | ||
| function PeriodicBC{E, P, C}(period::P) where {E, P, C} | ||
| E isa Symbol || error("PeriodicBC type parameter E must be a Symbol") | ||
| E in (:inclusive, :exclusive) || error("PeriodicBC type parameter E must be :inclusive or :exclusive") | ||
| return new{E, P}(period) | ||
| C isa Bool || error("PeriodicBC type parameter C must be a Bool") | ||
| return new{E, P, C}(period) | ||
| end |
There was a problem hiding this comment.
PeriodicBC now has three type parameters ({E,P,C}), but the surrounding docstring section (signature line and “Type Parameters” list above) still describes it as PeriodicBC{E,P} and doesn’t mention the new C/check parameter. Please update the docstring to match the actual type/constructor so generated docs and IDE help aren’t misleading.
Comment on lines
+66
to
71
| !!! note "Scaled near-zero endpoints" | ||
| `atol = 8eps` covers direct evaluations (e.g., `sin.(x)`), but not scaled | ||
| variants (e.g., `1e6 .* sin.(x)` where noise ≈ 1e6·eps). For those cases, | ||
| set `y[end] = y[1]` explicitly. | ||
|
|
||
| Throws `ArgumentError` if endpoints differ. |
There was a problem hiding this comment.
In the docstring note on “Scaled near-zero endpoints”, the guidance currently only says to set y[end] = y[1] explicitly, but the PR also introduces PeriodicBC(check=false) as the supported opt-out for this case. Consider mentioning check=false here as well to keep the docstring consistent with the new API and the updated error message tip.
Suggested change
| !!! note "Scaled near-zero endpoints" | |
| `atol = 8eps` covers direct evaluations (e.g., `sin.(x)`), but not scaled | |
| variants (e.g., `1e6 .* sin.(x)` where noise ≈ 1e6·eps). For those cases, | |
| set `y[end] = y[1]` explicitly. | |
| Throws `ArgumentError` if endpoints differ. | |
| !!! note "Scaled near-zero endpoints" | |
| `atol = 8eps` covers direct evaluations (e.g., `sin.(x)`), but not scaled | |
| variants (e.g., `1e6 .* sin.(x)` where noise ≈ 1e6·eps). For those cases, | |
| either set `y[end] = y[1]` explicitly, or construct the boundary condition as | |
| `PeriodicBC(check=false)` to opt out of this validation when you know the data | |
| are already periodic. | |
| Throws `ArgumentError` if endpoints differ. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 96.46% 96.41% -0.06%
==========================================
Files 103 103
Lines 8919 8944 +25
==========================================
+ Hits 8604 8623 +19
- Misses 315 321 +6
🚀 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
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 strict
==in periodic endpoint validation withisapprox+atol = 8eps(T)noise floor. AddPeriodicBC(check=false)to skip validation for scaled data.Motivation
Design
Endpoint dispatch
AbstractFloatisapprox8eps(T)(compile-time folded)Complex{<:AbstractFloat}isapprox8eps(real(T))_PromotableValueisapprox==PeriodicBC(check=false)For scaled data (
1e6 .* sin.(x)) where noise exceeds8eps.checkis a type parameter (not field) —PeriodicBCstays zero-size singleton, no allocation regression.